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 tha
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 enoug
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,
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
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, th
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
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
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
http
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,
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&id=13049
REVISION D
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-
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,
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,
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, #framewor
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
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];
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
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/
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:/
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 rig
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 D
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, #framewor
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
- usin
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, #ktexted
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.
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, he
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
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 setP
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,
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&id=12185
REVISION DETAIL
https://phabricator.kde
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 actu
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, kfu
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
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 QSc
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 inste
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 des
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
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 w
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/email
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 i
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
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, palan
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, thi
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://ph
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 boo
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 o
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 -> dataToSav
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&id=11990
REVISION DETAIL
https://phabricator.kde.org/D4847
A
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 DET
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 an
50 matches
Mail list logo
