D4847: KAuth integration in document saving
martinkostolny added a comment. I've created a follow-up diff https://phabricator.kde.org/D5394 and added every subscriber from here. I hope it wasn't too invasive of me. > Once implemented, it will be possible to get kauth for free when calling (for example) KIO::move(). Not sure if that's enough for what this patch does, though. I'm not sure about this. Maybe it can be used but I think we now want more atomic way of saving the file contents then saving in user's space and then moving to privileged location with KIO. But I can be missing something. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: elvisangelaccio, aacid, ivan, lbeltrame, fvogt, apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
elvisangelaccio added a comment. In https://phabricator.kde.org/D4847#101163, @aacid wrote: > How is this related with https://phabricator.kde.org/T5202 ? Once implemented, it will be possible to get kauth for free when calling (for example) `KIO::move()`. Not sure if that's enough for what this patch does, though. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: elvisangelaccio, aacid, ivan, lbeltrame, fvogt, apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
aacid added a comment. How is this related with https://phabricator.kde.org/T5202 ? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: aacid, ivan, lbeltrame, fvogt, apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
lbeltrame added a comment. Notice from downstream integration: this feature is considered an "abuse" of the Polkit system and at least openSUSE will explicitly disable it. See https://bugzilla.suse.com/show_bug.cgi?id=1033055 for the reasoning. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
lbeltrame added a comment. In https://phabricator.kde.org/D4847#101088, @ivan wrote: > @lbeltrame > > I don't really see 'reasoning' in that report more than something that sounds like 'polkit is a hack to replace suid for setting the system time and time zone'. That said, the inclusion of the feature is vetoed for now... We'll try to convince them otherwise, but not before the issues mentioned in this review are fixed. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: ivan, lbeltrame, fvogt, apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
fvogt added a comment. In https://phabricator.kde.org/D4847#101088, @ivan wrote: > @lbeltrame > > I don't really see 'reasoning' in that report more than something that sounds like 'polkit is a hack to replace suid for setting the system time and time zone'. Me neither. In my opinion this approach here is the most secure and also user friendly option possible, once the issues I found are fixed properly. We'll raise the issue with the security team again and try to convince them. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: ivan, lbeltrame, fvogt, apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
fvogt added a comment. Adding to Luca's comment, I also find two additional issues with this approach, it is absolutely impossible to make this secure. There is always a race condition between acquiring the privilege and renaming the file to the new location. Only solution for that is to pass the full file content to the helper (which would then give the user a checksum of the full document). Additional race condition is for new files: They are moved into the directory first and only after that the permissions are set. This is not the right approach, it needs to be done like: - Create empty file with the right permissions and owner in the new path - Rename temp file to the new path Therefore two more NAKs from me. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: fvogt, apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
ivan added subscribers: lbeltrame, ivan. ivan added a comment. @lbeltrame I don't really see 'reasoning' in that report more than something that sounds like 'polkit is a hack to replace suid for setting the system time and time zone'. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: ivan, lbeltrame, fvogt, apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
cullmann added a comment. Thanks! REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
This revision was automatically updated to reflect the committed changes. Closed by commit R39:ae60880c5f9b: KAuth integration in document saving (authored by martinkostolny). REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=12941=13049 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES autotests/src/katetextbuffertest.cpp autotests/src/katetextbuffertest.h src/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer_p.h src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny added a comment. Sure, no problem :). REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
dhaumann added a comment. Could you wait with this commit until after the 5.33 tag? Then we have 1 month instead of 3 days to find and fix regressions. The tag will happen this Saturday. :) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
dfaure accepted this revision. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor, dfaure Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny marked 3 inline comments as done. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12941. martinkostolny added a comment. Updating diff with refinements based on David's insights, thanks David! Regarding static slot, it was quite convenient to call it statically in unit test mode and it works. So I left it there like this. I have no problems making the slot non-static if necessary. REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=12309=12941 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES autotests/src/katetextbuffertest.cpp autotests/src/katetextbuffertest.h src/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer_p.h src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
dfaure added a comment. Looks good to me. Just some minor things I noticed. INLINE COMMENTS > katesecuretextbuffer.cpp:26 > +#include > +#include > +#endif where is stdio.h used? > katesecuretextbuffer.cpp:100 > +} > +const qint64 len = 4096; > +char buffer[len]; make this static so that the next line doesn't look to the compiler like a variable-size array (which isn't standard, although I'm not sure about C++11). Maybe it has to be a #define even, I'm not sure. > katesecuretextbuffer_p.h:21 > + > +#ifndef KATE_SECURE_TEXTBUFFER_H > +#define KATE_SECURE_TEXTBUFFER_H _P_H to match the header filename > katesecuretextbuffer_p.h:83 > + */ > +static ActionReply savefile(const QVariantMap ); > + A static slot is creative ;-) But if it works, that's fine. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny added a comment. > 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: 1. First try to open QSaveFile, if succeeded -> finish writing as before the patch 2. If opening QSaveFile fails KAuth action is called for creation of a temporary file in the same directory as the original target file 3. Then writing to this file is performed as regular user (same as before the patch) 4. Finally, second KAuth action is called to atomically rename the temporary file Owner and group is taken care of. Atomic rename is used only for Unix. On Windows there is a fallback using another QSaveFile which is also atomic when renaming but there is otherwise useless file copy beforehand. From my (non-expert) point of view this fallback is the only thing that needs fixing right now. But I currently cannot do that, it seems to me that it can be done later, too. > I am no security expert. Me neither. OK let's wait for somebody better qualified for this task :). REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
cullmann added a comment. I am no security expert, therefore I might not see obvious security flaws. Beside that we still have issues with the atomicity of the rename, it should work as it is now, or I am mistaken? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny added a comment. 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? Also if I'm too rookie for this, feel free to commandeer the revision. :) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12309. martinkostolny added a comment. 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: 1. Create temporary file right next to target file, set current user as owner, so it is writable without root privileges. 2. And after storing file contents outside kauth helper binary. atomically rename the temporary file. Any objections against using 2 actions? KAuth action session ensures that user is asked for password only first time using a KAuth action. I've used QTemporaryFile(filename) (instead of "xxx.tmp") so Qt took care of uniqueness of filename. Another note: I didn't call windows specific atomic rename method because I cannot test it on my machine. So on windows QSaveFile fallback is used for now. REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=12272=12309 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES autotests/src/katetextbuffertest.cpp autotests/src/katetextbuffertest.h src/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer_p.h src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
dfaure added a comment. I don't know the full architecture of this kauth stuff, but isn't it possible to save to .tmp rather than /tmp, ~/.cache or anywhere else? Then instead of "more likely" we are *sure* it's on the same device as the destination. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny marked 7 inline comments as done. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12272. martinkostolny added a comment. Thanks a sorry for all the rookie mistakes. I tried to fix the problems You mentioned: - QT->Qt - 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?) Now, while discussing the atomic rename, I'm throwing a thought: how about using QStandardPaths::CacheLocation (~/.cache) for the temporary file? It should always exist, it is more likely the same FS device and should be writable by the user. I did that in this diff and left a fallback for using QSaveFile if atomic rename fails. REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=12251=12272 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES autotests/src/katetextbuffertest.cpp autotests/src/katetextbuffertest.h src/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer_p.h src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
apol added inline comments. INLINE COMMENTS > dfaure wrote in katetextbuffer.cpp:904 > use .insert() to avoid the creation of a temporary. Or better use an initializer list? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: apol, dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
dfaure added a comment.
Right the only way to get atomic renaming is to write the tempfile next to
its final destination, NOT in /tmp.
(just like QSaveFile does ;)
INLINE COMMENTS
> katesecuretextbuffer.cpp:74
> +/**
> + * There is no atomic rename operation publicly exposed by QT.
> + *
It's Qt, not QT.
QT is QuickTime ;)
> katesecuretextbuffer.cpp:88
> +char buffer[len];
> +QDataStream dataStreamFrom();
> +QDataStream dataStreamTo();
You definitely don't need QDataStream just to call read and write on QIODevices.
> katesecuretextbuffer.cpp:101
> +#ifndef Q_OS_WIN
> +// ensure that the file is written to disk
> +#ifdef HAVE_FDATASYNC
Not necessary if you use QSaveFile, which does this already.
But maybe the best solution here (to avoid the file copy) is a temp file that
you can atomically rename, so let's discuss that before you get rid of all the
stuff-that-QSaveFile-does ;)
> katetextbuffer.cpp:807
> +
> +#ifndef Q_OS_WIN
> +if (newFile) {
The thing about using Qt's setPermissions() is that it should work on Windows
too.
> katetextbuffer.cpp:815
> +// set original file's permissions
> +saveFile->setPermissions(QFile(filename).permissions());
> }
Doesn't QSaveFile do this?
> katetextbuffer.cpp:885
> #ifdef HAVE_FDATASYNC
> -fdatasync(saveFile.handle());
> +fdatasync(saveFile->handle());
> #else
(pre-existing) done by QSaveFile already
> katetextbuffer.cpp:904
> +QVariantMap args;
> +args[QLatin1String("sourceFile")] = saveFile->fileName();
> +args[QLatin1String("targetFile")] = filename;
use .insert() to avoid the creation of a temporary.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4847
To: martinkostolny, dhaumann, #ktexteditor
Cc: dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin,
davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny marked 4 inline comments as done. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12251. martinkostolny added a comment. Thanks a lot for all the thoughts and suggestions! I tried to work them in, but I need help with some of them. I'm struggling with the atomic rename. I still find Christoph's suggestion (save to temp file and then move it in helper) a good option to stick with. But then we are in many cases unable to atomically move the tempfile because tempfile is usually in tmpfs (RAM) -> different device then target file to save. So we can't even use std::rename for this. Exist & remove & rename are racy so I went for copying to the QSaveFile and then commit. It's not exactly a one-liner, is there a better way? I may very well be on a wrong path here. About the event loop evil. I agree it should be re-written with QT connects and I don't mind digging in the code. But if I understand the existing code correctly, I'd also have to rewrite at least KateBuffer::saveFile(), DocumentPrivate::saveFile() and DocumentPrivate::documentSaveCopyAs(). I'd like to suggest doing all this in a separate diff/review. Did I miss something else? REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=12185=12251 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES autotests/src/katetextbuffertest.cpp autotests/src/katetextbuffertest.h src/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer_p.h src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions To: martinkostolny, dhaumann, #ktexteditor Cc: dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
dfaure added inline comments.
INLINE COMMENTS
> katesecuretextbuffer.cpp:1
> +#include "katesecuretextbuffer.h"
> +
Missing copyright header
> katesecuretextbuffer.cpp:39
> +{
> +// QTemporaryFile sets permissions to 0600, so fixing this
> +if (newFile) {
Isn't it possible to call setPermissions on the QTemporaryFile instance instead?
> katesecuretextbuffer.cpp:61
> +
> +// remove target file if there is any
> +if (!newFile) {
Not sure it matters, but exists+remove+rename is racy. The file could be
deleted by another process after exists() and before remove() (which would then
fail) or the file could be created by another process after the call to
remove() (and then rename() would fail).
QSaveFile doesn't have these issues because it uses the POSIX rename() function
which is atomic (and replaces any existing files). Unfortunately QFile doesn't
expose such a method (one is supposed to use QSaveFile instead...).
> katesecuretextbuffer.h:1
> +#ifndef KATE_SECURE_TEXTBUFFER_H
> +#define KATE_SECURE_TEXTBUFFER_H
Missing copyright header
Assuming the file is not installed, I would name it _p.h (but I don't know if
the rest of this framework follows this convention - I think it's in the KF5
guidelines though).
> katesecuretextbuffer.h:11
> +
> +class SecureTextBuffer : public QObject
> +{
a bit of docu maybe?
> katetextbuffer.cpp:75
> , m_lineLengthLimit(4096)
> +, m_alwaysUseKAuthForSave(KTextEditor::EditorPrivate::unitTestMode() ?
> alwaysUseKAuth : false)
> {
Isn't it weird to ignore the constructor argument in some cases?
> katetextbuffer.cpp:912
> +KAuth::ExecuteJob *job = saveAction.execute();
> +ok = job->exec();
> +}
Nested event loops are evil. Any chance to make this asynchronous (connecting
to a slot instead, for anything that needs to be done after completion of the
job) ?
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4847
To: martinkostolny, dhaumann, #ktexteditor
Cc: dfaure, anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin,
davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
cullmann added a comment. Thanks, as my KAuth knowledge is very limited (aka 0), any other input on this? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12185. martinkostolny added a comment. Understood and agreed, kauth_ktexteditor_helper it is :). REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=12181=12185 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES autotests/src/katetextbuffertest.cpp autotests/src/katetextbuffertest.h src/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer.h src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions To: martinkostolny, dhaumann, #ktexteditor Cc: anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
cullmann added a comment. Thanks for in cooperating my advice! I think for the naming, we could just call it kauth_ktexteditor_helper. That makes clear what it is and with ktexteditor in the name, we will not have clashs, or? We should avoid new "kate.." names in the installation as actually the framework is really non-kate. The internal naming is just historical. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny marked 2 inline comments as done. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12181. martinkostolny added a comment. Thanks for your guidance and for having the patience with me. QScopedPointer was indeed very useful. One thing I've noticed - it seems there isn't any naming convention for KAuth helper binary. Sometimes it ends with "_helper", sometimes it has different separators like dashes or non separators at all. Shouldn't this binary at least start with proper namespace like "org.kde.ktexteditor" to prevent future collisions? REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=12177=12181 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES autotests/src/katetextbuffertest.cpp autotests/src/katetextbuffertest.h src/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer.h src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions To: martinkostolny, dhaumann, #ktexteditor Cc: anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
anthonyfieroni added inline comments. INLINE COMMENTS > katetextbuffer.cpp:791 > */ > -QSaveFile saveFile(filename); > -saveFile.setDirectWriteFallback(true); > +QFileDevice *saveFile = new QSaveFile(filename); > +static_cast(saveFile)->setDirectWriteFallback(true); use QScopedPointer, remove manual delete > katetextbuffer.cpp:886 > #else > fsync(saveFile.handle()); > #endif Should be -> instead of . REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: anthonyfieroni, cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12177. martinkostolny added a comment. Good point, thanks! I now the helper is really light-weight. Helper is now only moving a temporary file and setting permissions/owner. I've also added a unit test. It directly calls the action method instead of calling KAuth action. It's not nice but I think it's safe and it works. REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=12084=12177 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES autotests/src/katetextbuffertest.cpp autotests/src/katetextbuffertest.h src/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer.h src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions To: martinkostolny, dhaumann, #ktexteditor Cc: cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
cullmann added a comment. Hi, I think this is a great thing to have! My biggest complaint ATM is: I would not move the saving code out to the helper binary, instead, it should stay in the place it is and we could just save to a tmpfile and tell the helper to move that file over the destination. With that, the with higher privileges running application is simpler (e.g. no need for filterdev which might execute complex compression libs) and we would avoid to store the complete file once more in memory. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: cullmann, ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny added a comment. 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. If somebody with better KAuth knowledge would know how to allow running otherwise secured KAuth action in unit-test runtime, that would be really helpful :). REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 To: martinkostolny, dhaumann, #ktexteditor Cc: ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, cullmann, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12084.martinkostolny added a comment. View RevisionAfter 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. Now to the autotests, I didn't make much progress, yet :). Maybe these will be a bit rookie questions but: Do you know of any KDE project I can learn from? A project with autotests that include testing KAuth action? I mean can I even test that root-owned file will only be saved with elevated privileges through KAuth action execution?REPOSITORYR39 KTextEditorCHANGES SINCE LAST UPDATEhttps://phabricator.kde.org/D4847?vs=12058=12084REVISION DETAILhttps://phabricator.kde.org/D4847AFFECTED FILESsrc/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer.h src/buffer/katetextbuffer.cpp src/buffer/org.kde.ktexteditor.katetextbuffer.actionsEMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: martinkostolny, dhaumann, KTextEditorCc: ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, Frameworks, head7, cullmann, kfunk, sars
D4847: KAuth integration in document saving
martinkostolny added a comment. View RevisionI think that leaving it as it is is not a solution In that case, I favour the second option (direct write to file).REPOSITORYR39 KTextEditorREVISION DETAILhttps://phabricator.kde.org/D4847EMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: martinkostolny, dhaumann, KTextEditorCc: ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, Frameworks, head7, cullmann, kfunk, sars
[Diff] D4847: KAuth integration in document saving
ltoscano added a comment. View Revision In D4847#91810, @martinkostolny wrote: This diff includes preserving owner and group when file is saved with elevated privileges. Actually loosing owner was (and is) already happening before my patch when user has the permission to write the file and is not its owner. In this case, I believe we have 3 options: Leave it as it is I've seen a downstream bug which could be about the same issue (https://bugzilla.redhat.com/show_bug.cgi?id=1143965) so I think that leaving it as it is is not a solution, if you can find a way to prevent this from happening.REPOSITORYR39 KTextEditorREVISION DETAILhttps://phabricator.kde.org/D4847EMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: martinkostolny, dhaumann, KTextEditorCc: ltoscano, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, Frameworks, head7, cullmann, kfunk, sars
[Diff] D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12058.martinkostolny added a comment. View RevisionNext iteration, still without autotests, I need to study them more. This diff includes preserving owner and group when file is saved with elevated privileges. Actually loosing owner was (and is) already happening before my patch when user has the permission to write the file and is not his owner. In this case, I believe we have 3 options: Leave it as it is Detect different owner and in such situation use direct writing (without QSaveFile) Use QSaveFile but in case of different owner ask for elevated privilege to be able to reset the owner Is there another solution I'm missing? PS: Regarding windows and other non-linux platforms I unfortunately cannot test the code there right now.REPOSITORYR39 KTextEditorCHANGES SINCE LAST UPDATEhttps://phabricator.kde.org/D4847?vs=12054=12058REVISION DETAILhttps://phabricator.kde.org/D4847AFFECTED FILESsrc/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer.h src/buffer/katetextbuffer.cpp src/buffer/org.kde.ktexteditor.katetextbuffer.actionsEMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: martinkostolny, dhaumann, KTextEditorCc: dhaumann, graesslin, davidedmundson, palant, kwrite-devel, Frameworks, head7, cullmann, kfunk, sars
[Diff] D4847: KAuth integration in document saving
martinkostolny marked 2 inline comments as done. View RevisionREPOSITORYR39 KTextEditorREVISION DETAILhttps://phabricator.kde.org/D4847EMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: martinkostolny, dhaumann, KTextEditorCc: dhaumann, graesslin, davidedmundson, palant, kwrite-devel, Frameworks, head7, cullmann, kfunk, sars
[Diff] D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 12054.martinkostolny added a comment. View RevisionThanks for all the feedback! I'm updating the diff to reflect most Dominik's points. I'll learn from them next time :). Anyway I was unable to remove the KAuth namespace. It doesn't work without it, this is also stated in KAuth documentation - e.g. here https://api.kde.org/frameworks/kauth/html/namespaceKAuth.html Please note that due to QMetaObject being picky about namespaces, you NEED to declare the return type as ActionReply and not KAuth::ActionReply. But this diff-update is not final, yet. I'm now working on the unit test and I need to address the issue mentioned by Martin - preserving file permissions when owner and group of edited file is not root.REPOSITORYR39 KTextEditorCHANGES SINCE LAST UPDATEhttps://phabricator.kde.org/D4847?vs=11991=12054REVISION DETAILhttps://phabricator.kde.org/D4847AFFECTED FILESsrc/CMakeLists.txt src/buffer/katesecuretextbuffer.cpp src/buffer/katesecuretextbuffer.h src/buffer/katetextbuffer.cpp src/buffer/org.kde.ktexteditor.katetextbuffer.actionsEMAIL PREFERENCEShttps://phabricator.kde.org/settings/panel/emailpreferences/To: martinkostolny, dhaumann, KTextEditorCc: dhaumann, graesslin, davidedmundson, palant, kwrite-devel, Frameworks, head7, cullmann, kfunk, sars
[Differential] [Commented On] D4847: KAuth integration in document saving
lbeltrame added a comment. > - Does that affect other platforms such as Windows in any way? I.e., does KAuth exist on Windows? KAuth had backends for OSX and Windows. Whether they are working or just stubs however, I don't know. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: martinkostolny, #ktexteditor, dhaumann Cc: lbeltrame, dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, cullmann, kfunk, sars
[Differential] [Requested Changes] D4847: KAuth integration in document saving
dhaumann requested changes to this revision.
dhaumann added a comment.
This revision now requires changes to proceed.
This is a good patch.
My main concerns are:
- please use const for variables that do not change anymore
- please don't use const & for primitive data types (like bool) in function
arguments, that does not make sense
- naming conventions: Instead of TextBufferSecure, the word SecureTextBuffer
sounds much more natural. Similarly, the header file can be renamed.
Could you provide an updated revision?
Some general questions
- Does that affect other platforms such as Windows in any way? I.e., does
KAuth exist on Windows?
- The text buffer itself is unit tested pretty well. Is it possible to have a
unit test for this? PS: We already have a " bool
KTextEditor::EditorPrivate::unitTestMode()" function that we could use to
virtually trigger the KAuth case somehow, not sure.
INLINE COMMENTS
> katetextbuffer.cpp:768-769
> +// prepare parameters for calling helper method
> +QString textCodec = QString::fromLatin1(m_textCodec->name());
> +int eolMode = static_cast(endOfLineMode());
> +QList dataToSave;
const QString textCodec = ...
const int eolMode = ...
Please use const whenever possible for locally defined variables.
> katetextbuffer_secure.cpp:1
> +#include "katetextbuffer_secure.h"
> +#include "katetextline.h"
I would prefer katesecuretextbuffer.h as filename. Intermixing underscrores is
rather uncommon in KDE's sources.
> katetextbuffer_secure.cpp:26-31
> +QString filename = args[QLatin1String("filename")].toString();
> +QString mimeTypeForFilterDev =
> args[QLatin1String("mimeTypeForFilterDev")].toString();
> +QString textCodec = args[QLatin1String("textCodec")].toString();
> +bool generateByteOrderMark =
> args[QLatin1String("generateByteOrderMark")].toBool();
> +int endOfLineMode = args[QLatin1String("endOfLineMode")].toInt();
> +bool newLineAtEof = args[QLatin1String("newLineAtEof")].toBool();
Please prepend 'const' for all local variables whenever possible.
> katetextbuffer_secure.cpp:34
> +
> +bool ok = saveInternal(filename, mimeTypeForFilterDev, textCodec,
> generateByteOrderMark, endOfLineMode, newLineAtEof, dataToSave);
> +
const bool ok = ...
> katetextbuffer_secure.cpp:87
> +// just dump the lines out ;)
> +int lineCount = dataToSave.count();
> +for (int i = 0; i < lineCount; ++i) {
const int lineCount = ...
> katetextbuffer_secure.h:10
> +
> +using namespace KAuth;
> +
Please no using namespace KAuth here, especially since this is a header file.
> katetextbuffer_secure.h:12
> +
> +class TextBufferSecure : public QObject
> +{
Please call it SecureTextBuffer, that feels much more natural than adding the
suffix "Secure".
> katetextbuffer_secure.h:18
> +
> +TextBufferSecure() {};
> +
Pedantic: no semicolon after a closing function brace, i.e.:
SecureTextBuffer() {}
> katetextbuffer_secure.h:22
> +
> +bool saveInternal(const QString , const QString
> , const QString , const bool
> ,
> + const int , const bool ,
> const QList );
Please never use const references for primitive types. Here:
- const bool & --> bool
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D4847
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: martinkostolny, #ktexteditor, dhaumann
Cc: dhaumann, graesslin, davidedmundson, palant, kwrite-devel, #frameworks,
head7, cullmann, kfunk, sars
[Differential] [Changed Subscribers] D4847: KAuth integration in document saving
graesslin added subscribers: davidedmundson, graesslin. graesslin added a comment. I really like what I see here! Just a thought: what happens if the file is not owned by root, but e.g. by www-data? If I understand the code correctly it might change to be owned by root due to the usage of QSaveFile? I would like to see some KAuth experts (e.g. @davidedmundson ) to have a good look at the code to verify that it doesn't introduce vulnerabilities. As far as I studied the code I didn't see anything bad. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: martinkostolny, #ktexteditor Cc: graesslin, davidedmundson, palant, kwrite-devel, #frameworks, head7, cullmann, kfunk, sars, dhaumann
[Differential] [Updated, 318 lines] D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 11991. martinkostolny added a comment. 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 REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=11990=11991 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES src/CMakeLists.txt src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer_secure.cpp src/buffer/katetextbuffer_secure.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: martinkostolny, #ktexteditor Cc: palant, kwrite-devel, #frameworks, head7, cullmann, kfunk, sars, dhaumann
[Differential] [Updated, 318 lines] D4847: KAuth integration in document saving
martinkostolny updated this revision to Diff 11990. martinkostolny added a comment. I missed this one - thanks, Wladimir. Fixed. REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D4847?vs=11974=11990 REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES src/CMakeLists.txt src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer_secure.cpp src/buffer/katetextbuffer_secure.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: martinkostolny, #ktexteditor Cc: palant, kwrite-devel, #frameworks, head7, cullmann, kfunk, sars, dhaumann
[Differential] [Changed Subscribers] D4847: KAuth integration in document saving
palant added inline comments. INLINE COMMENTS > org.kde.ktexteditor.katetextbuffer.actions:8 > +Name=Save Document > +Description=Root privileges are needed save this document > +Policy=auth_admin You probably meant to write "**to** save this document" REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: martinkostolny, #ktexteditor Cc: palant, kwrite-devel, #frameworks, head7, cullmann, kfunk, sars, dhaumann
[Differential] [Request, 318 lines] D4847: KAuth integration in document saving
martinkostolny created this revision. martinkostolny added a project: KTextEditor. Restricted Application added subscribers: Frameworks, kwrite-devel. Restricted Application added a project: Frameworks. REVISION SUMMARY Before this patch: if one opens a write protected document, makes changes and then wants to save, error message occurs about insufficient privileges or disk space. With this patch kate-part will try to save the document contents with elevated privileges in case the regular save failed. So that KAuth graphical prompt is presented to user. I believe this was suggested by many KDE users as a wanted feature. Please let me know if this isn't the right approach. I'm in fact quite new to KTextEditor as well as KAuth so feel free to criticize the code. I'll try to fix everything you point at :). What I basically did: - created TextBufferSecure class (for dedicated KAuth helper binary) - moved most contents of TextBuffer:save() method to this new class' saveInternal() method - TextBuffer:save() method now first tries to save with TextBufferSecure::saveInternal() helper method - if that fails it will call it again through KAuth action TEST PLAN - editing & saving document in home page - editing & saving /etc/hosts - tried with Krusader's "internal viewer" which uses ReadWritePart + tried with Kate But I'm sure there are a lot of other use-cases I didn't think of. Maybe you think of somehting. On my mind is now saving documents with different encoding, eol type, saving huge documents. I'll do that and if problems occur, I'll fix the diff. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D4847 AFFECTED FILES src/CMakeLists.txt src/buffer/katetextbuffer.cpp src/buffer/katetextbuffer_secure.cpp src/buffer/katetextbuffer_secure.h src/buffer/org.kde.ktexteditor.katetextbuffer.actions EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: martinkostolny, #ktexteditor Cc: kwrite-devel, #frameworks, head7, cullmann, kfunk, sars, dhaumann
