Add privilegeExecution field to file protocol description
AbandonedPublic

Authored by cblack on Aug 26 2017, 5:45 PM.

Details

Reviewers
dfaure
chinmoyr
sitter
ngraham
Group Reviewers
Frameworks
Maniphest Tasks
T6561: Polkit support in KIO
Summary

"privilegeExecution" field will specify if protocol can perform file operation with elevated privileges.

Dolphin relies on KFileItemListProperties for enabling some of its context menu actions. Internally KFileItemListProperties uses KProtocolManager to determine if a location supports reading, writing,
deleting and moving. With these changes KProtocolManager will now check for the said field. Consequently, dolphin will be aware of our new file protocol and will show its context menu without any
action being disabled.

BUG: 179678

Test Plan
[compile patch to your install location; for me it's ~/kde/usr]

source ~/kde/build/kio/prefix.sh

export KDE_FORK_SLAVES=1

sudo cp ~/kde/usr/share/polkit-1/actions/org.kde.kio.file.policy /usr/share/polkit-1/actions/

sudo cp ~/kde/usr/share/dbus-1/system-services/org.kde.kio.file.policy /usr/share/dbus-1/system-services/

sudo cp ~/kde/usr/share/dbus-1/system.d/org.kde.kio.file.policy /usr/share/dbus-1/system.d/

sudo cp ~/kde/usr/lib64/libexec/kauth/file_helper /usr/lib64/libexec/kauth/

kdeinit5

dolphin [then navigate to / and try to create a file or folder]

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25359
Build 25377: arc lint + arc unit
chinmoyr created this revision.Aug 26 2017, 5:45 PM
dfaure accepted this revision.Sep 3 2017, 8:09 AM
dfaure added inline comments.
src/core/kprotocolmanager.h
479

Missing @since 5.39

This revision is now accepted and ready to land.Sep 3 2017, 8:09 AM

What's the status of this patch?

What's the status of this patch?

I will push this patch after fixing all security issues with kauth support in KIO. (T8075)

mati865 added a subscriber: mati865.Jun 3 2018, 1:52 PM
mreeves added a subscriber: mreeves.Jun 4 2018, 3:47 AM

The outstanding security issues have been resolved (see T8075). We have requested a re-review from the SUSE security team, but have not received it yet. Given that the original schedule for this feature has already slipped by almost two years, I would like to propose that we land this patch and turn it on, and resolve any newly-discovered issues in follow-up work.

Any objections?

The outstanding security issues have been resolved (see T8075). We have requested a re-review from the SUSE security team, but have not received it yet. Given that the original schedule for this feature has already slipped by almost two years, I would like to propose that we land this patch and turn it on, and resolve any newly-discovered issues in follow-up work.

Any objections?

+1

@chinmoyr phab reports that this has a missing dependency/patch base. Is there some unmerged patch that's left to do, or does it just need a rebase on master?

chinmoyr updated this revision to Diff 80341.Apr 17 2020, 3:07 AM
chinmoyr marked an inline comment as done.

Rebased on master.

Restricted Application added a project: Frameworks. · View Herald TranscriptApr 17 2020, 3:07 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham commandeered this revision.Apr 17 2020, 3:54 AM
ngraham added a reviewer: chinmoyr.

Yoink!

ngraham updated this revision to Diff 80342.Apr 17 2020, 3:55 AM
  • Fix build failure
  • Add more stuff (thanks @chinmoyr)
ngraham updated this revision to Diff 80343.Apr 17 2020, 3:56 AM

Set the patch base

chinmoyr requested changes to this revision.Apr 17 2020, 4:02 AM
chinmoyr added inline comments.
src/ioslaves/file/file_unix.cpp
1331 ↗(On Diff #80343)

We need to set a different message for each action.

This revision now requires changes to proceed.Apr 17 2020, 4:02 AM

[insert I-have-no-idea-what-I'm-doing dog meme here]

When trying to create items in root-owned locations, I'm getting an errors saying "The process for the file protocol died unexpectedly." or else Dolphin simply crashes with a totally unhelpful backtrace.

ngraham updated this revision to Diff 80644.Apr 20 2020, 1:06 PM

Install auth helper too

ngraham updated this revision to Diff 80668.Apr 20 2020, 5:09 PM

Maybe fix a thing

[insert I-have-no-idea-what-I'm-doing dog meme here]

When trying to create items in root-owned locations, I'm getting an errors saying "The process for the file protocol died unexpectedly." or else Dolphin simply crashes with a totally unhelpful backtrace.

If it's still happening, please have a look at https://community.kde.org/Guidelines_and_HOWTOs/Debugging/Debugging_IOSlaves

feverfew added a comment.EditedApr 20 2020, 10:50 PM

[insert I-have-no-idea-what-I'm-doing dog meme here]

When trying to create items in root-owned locations, I'm getting an errors saying "The process for the file protocol died unexpectedly." or else Dolphin simply crashes with a totally unhelpful backtrace.

If it's still happening, please have a look at https://community.kde.org/Guidelines_and_HOWTOs/Debugging/Debugging_IOSlaves

I managed to find and fix one of the crashes (not sure if it's the same one that @ngraham had).

However, even with this patch I can't get it to work, I notice this in the debug output: kf5.kauth tried to start an invalid action

This sounds to me like I'm testing wrong but I can't tell what... How does one properly test KAuth actions?

ngraham edited the test plan for this revision. (Show Details)Apr 24 2020, 1:30 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)Apr 24 2020, 6:50 PM
ngraham edited the test plan for this revision. (Show Details)
kkong added a subscriber: kkong.May 2 2020, 5:35 PM
ngraham edited the summary of this revision. (Show Details)May 12 2020, 7:49 PM
sitter requested changes to this revision.Jun 5 2020, 2:39 PM
sitter added a subscriber: sitter.

This really cannot land right now IMHO. Dolphin can actually deadlock itself because it uses way too much nested event looping and will be entirely unresponsive to mouse inputs when certain timers happen to trigger. A trivial way to reproduce this is to try and duplicate a file in file:/

Also making folders is actually not even implemented as the relevant mkdirjob seems to lack privilege enablement. That's not strictly speaking blocking but renders the actual UX broken.
I've started a page for more comprehensive testing of the entire polkit epic.
https://invent.kde.org/frameworks/kio/-/wikis/PrivilegedOperations

I understand this diff is the last piece missing to enable the entire priviledge operations systems, so the entire shebang better be working first (:

This revision now requires changes to proceed.Jun 5 2020, 2:39 PM

This really cannot land right now IMHO. Dolphin can actually deadlock itself because it uses way too much nested event looping and will be entirely unresponsive to mouse inputs when certain timers happen to trigger. A trivial way to reproduce this is to try and duplicate a file in file:/

Interesting... Yes if so that's a serious blocker.

Also making folders is actually not even implemented as the relevant mkdirjob seems to lack privilege enablement. That's not strictly speaking blocking but renders the actual UX broken.

I can confirm that I noticed this too. I also noticed that sometimes the dialog wouldn't show at all because the slave kept state that said that permission was denied (and hence it believed showing the dialog was redundant) when in fact my permission was never asked. Although this only happened for mkdir for me IIRC. So the slave might be mishandling state. At the same time, I'm not sure if the slave is in a position to handle such state (they're short-lived?).

Creating new files I believe also asks permission twice (one for file.part and the other for file.txt), that comes across as bad UX for me.

cblack added a subscriber: cblack.Aug 5 2020, 10:49 PM

This really cannot land right now IMHO. Dolphin can actually deadlock itself because it uses way too much nested event looping and will be entirely unresponsive to mouse inputs when certain timers happen to trigger. A trivial way to reproduce this is to try and duplicate a file in file:/

Can't reproduce, duplication works fine.

cblack commandeered this revision.Aug 5 2020, 10:56 PM
cblack added a reviewer: ngraham.
cblack updated this revision to Diff 83342.Aug 5 2020, 11:03 PM
cblack marked an inline comment as done.

Add more descriptive authentication details

cblack updated this revision to Diff 83343.Aug 5 2020, 11:04 PM

Break those cases; this lang ain't got sane case behaviour

sitter added inline comments.Aug 6 2020, 8:53 AM
src/ioslaves/file/file_unix.cpp
1372 ↗(On Diff #83343)

I'd advise handling default cases. The compiler can no longer warn of unhandled enum values when default is used. Instead I'd convert the entire switch into a helper function static QString actionTypeToString(ActionType action) and in there switch like so:

switch (action) {
case ActionType::CHMOD:
return QStringLiteral("Authentication is required to change this file's permissions.");
case ActionType::CHOWN:
...
}
// any values not explicitly handled gets here making this the de-facto default handling
Q_UNREACHABLE()
return QString()
}

This then makes the code here less repetitive as the entire switch gets squashed down to details.insert(KAuth::Action::AuthDetail::DetailMessage, actionTypeToString(action));

cblack updated this revision to Diff 83344.Aug 6 2020, 8:31 PM

Redo that switchcase

cblack marked an inline comment as done.Aug 6 2020, 8:31 PM
ngraham added inline comments.Aug 7 2020, 4:27 PM
src/ioslaves/file/file_unix.cpp
1349 ↗(On Diff #83344)

Shouldn't these be localized?

Also can you change "directory" to "folder"?

Oh and thanks for taking this over, @cblack. :)

This really cannot land right now IMHO. Dolphin can actually deadlock itself because it uses way too much nested event looping and will be entirely unresponsive to mouse inputs when certain timers happen to trigger. A trivial way to reproduce this is to try and duplicate a file in file:/

Can't reproduce, duplication works fine.

It no longer deadlocks, it still doesn't work though. Now I get an empty file

dolphin(137764)/(kf5.kio.core.copyjob) KIO::copyAs: src= QUrl("file:///list") dest= QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::slotStart: CopyJob: stating the dest QUrl("file:///")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJob::slotResult: d->state= 1
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::slotResultStating: 
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::slotResultStating: dest is dir: true
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::statCurrentSrc: fast path! found info about QUrl("file:///list") in KCoreDirLister
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::addCopyInfoFromUDSEntry: fileName= "list" url= QUrl("file:///list")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::addCopyInfoFromUDSEntry: uSource= QUrl("file:///list") uDest(1)= QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::addCopyInfoFromUDSEntry:  uDest(2)= QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::addCopyInfoFromUDSEntry:   QUrl("file:///list") -> QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::sourceStated: Source is a file (or a symlink), or we are linking -> no recursive listing
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::statNextSrc: Setting m_dest to QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::statCurrentSrc: Stating finished. To copy: 39549 , available: 155277156352
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: 
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: preparing to copy QUrl("file:///list") 39549 155277156352
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: copying "/list copy"
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: Copying QUrl("file:///list") to QUrl("file:///list copy")
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::slotProcessedSize: 39549
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::slotProcessedSize: emit processedSize 39549
kio_file(137793)/(kf5.kio.kio_file) FileProtocol::copy: Could not change permissions for "/list copy"
kio_file(137793)/(kf5.kio.kio_file) FileProtocol::copy: Couldn't preserve group for "/list copy"
kio_file(137793)/(kf5.kio.kio_file) FileProtocol::copy: Couldn't preserve access and modification time for "/list copy"
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJob::slotResult: d->state= 6
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::slotResultCopyingFiles: 0 files remaining
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: 
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJobPrivate::copyNextFile: copyNextFile finished
dolphin(137764)/(kf5.kio.core.copyjob) KIO::CopyJob::emitResult: KDirNotify'ing FilesAdded QUrl("file:///")
cblack updated this revision to Diff 83347.Aug 11 2020, 9:18 PM
cblack marked an inline comment as done.

i18n the strings