Adding support for mounting KIOFuse URLs for applications that don't use KIO
ClosedPublic

Authored by feverfew on Aug 23 2019, 6:09 PM.

Details

Summary

This patch is required to provide seamless integration with KIOFuse and KIO.

The patch attempts to convert URLs that applications will not understand
natively into local paths based on a KIOFuse mount. If successful, the URL
passed to the application is changed to its location within the KIOFuse mount.
If it is not successful, the URL is not changed.

If KIOFuse is not installed, then kioexec is simply used.

FEATURE: 75324
BUG: 398216
BUG: 330192
CCBUG: 398446
FIXED-IN: 5.66

Test Plan

Compile KIO with this patch.

Build and install KIOFuse (follow instructions in README): https://invent.kde.org/kde/kio-fuse

Once KIO is built, Dolphin is built with this newly built KIO, and kio-fuse is built,
one should start the kio-fuse process.

The easiest way to test this is via Dolphin (that is built with this patch): dbus-launch dolphin.

Once you have done the above one can simply browse to any non-local location in
say, Dolphin, where upon opening a URL in a non-KIO enabled app, it should open
in the KIOFuse mount, except for MTP/Gdrive URLs - those will use KIOExec.
Conversely, KIO URLs in KIO-enabled apps should not use KIOFuse.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes

@feverfew thanks for rebasing https://invent.kde.org/kde/kio-fuse/merge_requests/2. I've tested it and confirmed that it fixes case #2! We can continue that portion of the view in the comments there.

feverfew updated this revision to Diff 70773.Dec 2 2019, 6:43 PM

Check for password instead of authority

This latest change made Totem work, but broke VLC and SMplayer for my password-protected Samba share. See the comment I left on https://invent.kde.org/kde/kio-fuse/merge_requests/2#note_19607.

feverfew updated this revision to Diff 70799.Dec 2 2019, 11:37 PM

Chek for userInfo instead of just password

I'm afraid that even with that change, the issue is still present. I honestly don't think it would be the worst thing in the world if we always handed the kio-fuse paths to apps that don't use ioslaves. My impression is that the few apps that have implemented their own samba clients and the like did so mostly to work around the lack of this feature.

fvogt added a comment.Dec 3 2019, 7:57 AM

I'm afraid that even with that change, the issue is still present. I honestly don't think it would be the worst thing in the world if we always handed the kio-fuse paths to apps that don't use ioslaves.

It would be. I like to have links like http://kde.org opened properly in the web browser, ftp://some/where opened in an FTP client and so on...
Media players know more about the format and streaming it than kio-fuse ever could, so avoiding layers in between if possible is definitely an advantage.

I haven't read everything in great detail... but...

Quick braindump of musing I did elsewhere: as far as credential hand-over is concerned this is likely a problem that needs a workaround for now as there is no clear cut solution that I am aware of. It may be worth talking to gnome about this and come up with an xdg standard for this.

The trouble is: we can't pass credentials in the URL as that'd be an exec argument and that would by default leak into /proc where the credential is then world readable. What we need is an additional system to pass credentials.

IMHO what likely is the smartest choice to is to have credentials stored in a running Secret Service API daemon and have clients then get the credentials out of there. Possibly with some additional ephemeral storage type (e.g. we cache credentials in kiod but store them in kwallet currently, that gives two points where we can leak secrets. if we were to move everything into the secret service we'd have a single service that needs securing). Just my opinion on the matter.

Until we have a cross desktop spec defining the passing of credentials I imagine we can't really let applications handle URLs for which we have/need credentials and I suspect determining if we need credentials is also a bit tricky.
The opposite extreme is to always pass when X-KDE-Protocols is set and assume that the applications are actually working correctly (e.g. vlc ought to talk to kiod/KPasswdServerClient to get credentials, otherwise its declaration of X-KDE-Protocols is incorrect and you are looking at a bug in vlc. at the very least it should throw up its own auth dialog if it doesn't know what to do).

feverfew updated this revision to Diff 70823.Dec 3 2019, 12:47 PM

Convert KIO URLs to KIOFuse URL is app is not KIO-enabled.

I'm afraid that even with that change, the issue is still present. I honestly don't think it would be the worst thing in the world if we always handed the kio-fuse paths to apps that don't use ioslaves.

It would be. I like to have links like http://kde.org opened properly in the web browser, ftp://some/where opened in an FTP client and so on...
Media players know more about the format and streaming it than kio-fuse ever could, so avoiding layers in between if possible is definitely an advantage.

If the URL is protected then we have to pass it to KIOFuse as most apps don't know how to get the credentials from kpasswdserver. This is a bug that has always existed with KIOExec, where before this patch it would always pass on the original URL to non-KIO apps if they understood them, but would have the same issue with credentials being required. From what I understand, userInfo() always seems to be emtpy even if it is password protected, so there's nothing clever I can do where if it isn't password protected I can just pass it straight to the app. It looks like we'll simply have to send the KIOFuse URL to all non-KIO apps. Trying to optimise I think is out of scope for this patch. I've blacklisted http/https seeming as they don't make much sense as a filesystem.

With the latest version of this patch, now every file always gets downloaded through kioexec on my password-protected Samba share; nothing ever gets the kio-fuse path. In fact the share never gets mounted at all when I try to open something on the share in a non-KDE app.

feverfew updated this revision to Diff 70830.Dec 3 2019, 3:30 PM

ignore scheme handler

Okay, that fixed it for the case of opening files in non-KDE apps on my Samba share. Now they open from the FUSE path--all of them! So this is good.

However now kioexec downloads http URLs opened from other apps instead of just passing them off to the browser. :(

So before this patch we had the following behaviour:

  1. If app is KIO-enabled, pass URLs unchanged.
  2. else for each URL, if app claims to support the protocol pass the original URL, otherwise change the URL to the corresponding KIOExec path.

This causes a problem as noted in 330192 and this blog post. In particular, if a non-KIO claims to support a certain protocol (e.g., VLC supports smb), then we won't convert to a KIOExec path, but we strip out the stuff that's in userInfo() (credentials) before sending it over.

What I previously tried to do was the following, in an attempt to close:

  1. If app is KIO-enabled, pass URLs unchanged.
  2. else for each URL, if app claims to support the protocol AND userInfo() is empty pass the original URL, otherwise change the URL to the corresponding KIOExec path.

However, that doesn't really help as apparently even if, say, a samba share is password protected, at the point I'm checking the password in resolveURLs() or resultingArguments() (truth be told, I'm not sure which, if not both, codepaths are being hit in our current testing), userInfo() is always empty, so this optimisation isn't possible.

So then one could just always direct non-KIO apps to use KIOFuse. Well that causes another problem, what about stuff like http(s)? It doesn't really make sense as a filesystem and we'd definitely like to forward it to the browser. So currently I can special case http/https. but is that the only one we want to special case?

Ideally, I'd like to know if there is a place where userInfo is only empty when there is genuinely no user/pass combo needed to access the resource at the URL. If so, then it can actually be possible to use KIOFuse on apps that support a URL only if it needs to have credentials (which it won't have) such that 330192 can be closed, without needing any hacks.

Thanks for that very clear and understandable synopsis. I'll let others figure out the best path forward. :)

feverfew updated this revision to Diff 70866.Dec 3 2019, 10:27 PM

don't send http(s) to kioexec

feverfew edited the summary of this revision. (Show Details)Dec 3 2019, 10:29 PM
ngraham accepted this revision.Dec 3 2019, 10:39 PM

All issues are fixed now for me! It works perfectly!๐ŸŽ‰๐ŸŽ‰

This revision is now accepted and ready to land.Dec 3 2019, 10:39 PM
alexde added a subscriber: alexde.Dec 4 2019, 10:44 AM
dfaure requested changes to this revision.Dec 4 2019, 11:45 AM

OK, please give me a day or two to review this, it shouldn't go in before Saturday anyway so we have one month of testing in git before release

This revision now requires changes to proceed.Dec 4 2019, 11:45 AM

If something is ready for full code review, can we remove "WIP:" from the title.

Can you please move the DBus XML file in a common location

I just tested writing today, for files opened in 3rd-party apps that get the FUSE mount path. Results:

  • Gedit: everything works
  • Blender: everything works
  • Inkscape: though the file is very small, the save operation is somewhat slow, but everything works
  • GIMP, File โ†’ Save: save operation hangs forever
  • GIMP, file> File โ†’ Overwrite: save operation is slow but completes. However, the overwritten file is corrputed. Here's the corrupt file:

The slow speed is maybe something worth investigating, but these could all be app-specific issues (especially GIMP).

fvogt added a comment.Dec 4 2019, 4:41 PM

I just tested writing today, for files opened in 3rd-party apps that get the FUSE mount path. Results:
...

Was that with or without the KIO::open merge request?

Issues with KIOFuse itself should be opened up as an issue, or as a reply to the MR that is being tested; this differential only concerns communication between the KIOFuse mount and KIO itself.

In particular, if you could test writing with both master and FileJobCombo (!2, i.e. the seeking MR) and tell us what happens that would be great. What slave were you testing? Does this also occur if you mount something from the file slave (e.g. file:///home/nate/image.xcf); instructions for mounting without this differential are available in the README of the KIOFuse repo. A log for both branches would be great (as you did earlier following fvogt's instructions). A reply to all of this in the relevant issue tracker or MR would be great. Thanks.

Was that with or without the KIO::open merge request?

With.

Anyway I guess this patch itself is fine now and the remaining issues are elsewhere. :)

In fact both issues I found seem to be caused by !2. See https://invent.kde.org/kde/kio-fuse/merge_requests/2#note_19771. We can continue the discussion there.

Why does KRun duplicate all of the (new) code from DesktopExecParser, when DesktopExecParser is actually a helper class for KRun?
I would expect it to have solved all this already, unless I'm missing something about the various code paths.
The code you changed in krun.cpp was supposed to simply resolve desktop:/foo to file:///bleh but everything else about %u/%f is done by DesktopExecParser.

src/core/desktopexecparser.cpp
311

, or KIOFuse

317

requests.reserve(d->urls.count());

321โ€“322

Minor: can you use count() (Qt-like) or size() (STL-like)? In 20 years I never saw code that uses length() for a vector or a list, it's just very unusual in Qt code :-)

[occurs twice]

322

Use d->urls.at(i) to avoid a detach given that d->urls isn't const here.

[occurs twice]

325

Could we have a kill switch for KIOFuse if it fails to work properly in some release?

E.g. some export KIO_FUSE=0 or export KIO_FUSE_DISABLE=1.
Or a KConfig entry (use KSharedConfig::openConfig()->group("KIO") for best performance and flexibility -- then it can be disabled for one calling application or for all)

331

"Struggle" sounds like the apps are limited/buggy.

But if what you mean is that the password (e.g. stored in kpasswdserver after the user typed it in the KDE dialog asking for it) won't be available to non-KIO applications, that seems logical and I do accept that argument. It's just the writing that is surprising.

332

The comment says "in case there is a password", the code doesn't.
Probably historical, please fix :)

On this topic I saw earlier comments in the merge request which I found confusing.
It's very rare for an application to have the password stored inside the URL itself. That would mean the user typing it in clear in a location bar or in a terminal, bad idea. Instead, the user typically types a URL like ftp://user@host/ and the kioslave asks for the password, and stores it in kpasswdserver.

Also you wrote "we strip out the stuff that's in userInfo()". (where QUrl::userInfo is username+password). But surely, while we might strip out passwords for security reasons, we never strip out the username, do we?

335

This duplicates line 344.

if (a) { if (!b) { foo } } else { foo }
is equivalent to
if (!a || !b) { foo }

In your case a is http || https which makes this a big uglier, but you could use what other pieces of code do and write url.scheme().startsWith(QLatin1String("http")).

So this becomes if (!url.scheme().startsWith(QLatin1String("http")) || !supportedProtocols(d->service).contains(url.scheme())) {

345

auto &request to avoid copying

346

This blocks.

I don't mind much myself, but I know some people had a mandate to remove as many blocking calls as possible from KRun and related code. Or was that only avoiding blocking I/O because of network mounts? [which this patch is all about adding more of...]. @broulik ?

368

This nesting under else is unnecessary given the return just above. It's even a bit confusing that some code is in else and then some code is outside the if/else, but the '}' could be anywhere in between, that would make no difference.

I would suggest to just remove the else.

371

for (const auto & request : qAsConst(requests))

or use std::vector if you find qAsConst ugly :-)

src/widgets/CMakeLists.txt
80 โ†—(On Diff #70866)

Just use ../core/org.kde.KIOFuse.VFS.xml instead of duplicating the file.

But even better would be to not need it in KRun, see separate comment.

src/widgets/krun.cpp
581 โ†—(On Diff #70866)

count or size

582 โ†—(On Diff #70866)

.at(i)

broulik added inline comments.Dec 5 2019, 11:04 AM
src/core/desktopexecparser.cpp
346

Blocking IO is worse than blocking DBus, still not nice, especially given the "job"-like nature of KRun I would expect it to be fully async.

Thanks for the reviews everyone! I'll update the diff and respond to the comments no later than Tuesday

The opposite extreme is to always pass when X-KDE-Protocols is set and assume that the applications are actually working correctly (e.g. vlc ought to talk to kiod/KPasswdServerClient to get credentials, otherwise its declaration of X-KDE-Protocols is incorrect and you are looking at a bug in vlc. at the very least it should throw up its own auth dialog if it doesn't know what to do).

There's a mistake in this reasoning. The "KDE" in X-KDE-Protocols only means that this is a KDE-specific field in desktop files, it wasn't standardized.
It doesn't however mean that these protocols are implemented using KIO.
In fact KIO-based apps normally just set X-KDE-Protocols=KIO. Other values for that field (i.e. an actual list of protocols) *is* what other apps are supposed to use there.

But yes, this doesn't solve the credentials issue.
I like the suggestion in another comment to implement that in SecretService... (which could replace kpasswdserver then, at least for the storage part, only the GUI would remain).

feverfew added a comment.EditedDec 12 2019, 7:20 PM

Why does KRun duplicate all of the (new) code from DesktopExecParser, when DesktopExecParser is actually a helper class for KRun?
I would expect it to have solved all this already, unless I'm missing something about the various code paths.
The code you changed in krun.cpp was supposed to simply resolve desktop:/foo to file:///bleh but everything else about %u/%f is done by DesktopExecParser.

Using git grep it seems to me that KRun::resolveURLs is called in KRun::runApplicationImpl and KRun::runService but KIO::DesktopExecParser::resultingArguments isn't called in those two functions. If URL conversion isn't necessary in those functions then I'll happily get rid of the code.

feverfew added inline comments.Dec 12 2019, 7:38 PM
src/core/desktopexecparser.cpp
325

One can always remove/uninstall kiofuse if it doesn't work or if they don't really want the feature?

332

From what others have tested userInfo() is always empty even when the URL is actually a protected Samba share... This means I can't know if a certain URL is likely to have a password or not, I just have to assume it does. I'll check again just in case, as indeed it does seem odd, but that was my conclusion.

346

In these methods I have to return the URLs within the function, so blocking is inevitable. If I could return the URLs via a signal then blocking could be avoided.

feverfew updated this revision to Diff 71423.Dec 13 2019, 12:02 PM
  • Merge branch 'master' into arcpatch-D23384
  • Address comments
  • Remove unnecessary mount requests in krun
  • Only use KIOFuse if password is empty

On further looking, it seems like git grep doesn't really tell the full picture. It seems like resultingArguments is called before resolveURLs is, so I've simplified the diff as requested.

src/core/desktopexecparser.cpp
332

From my own testing your intuition seems correct. I've now amended my diff appropriately

feverfew marked 9 inline comments as done.Dec 13 2019, 2:56 PM

The current patch should be tested with latest kio-fuse master, as the blacklisting has been moved there to keep this patch clean.

dfaure requested changes to this revision.Dec 13 2019, 10:06 PM

For SMB shares I guess what can happen is that the URL doesn't have a username in it, but it still needs a password? IIRC there was no username in "old" samba (W95, W98, XP, ...?).
This might explain what you were saying about people having password problems even with empty userInfo() (which means username and password).
But we can hardly know, just by looking at a URL, if a password is needed... I guess most of time a password is actually needed? Maybe you're right after all, and we should always send SMB urls to kiofuse..... Dunno.

src/core/desktopexecparser.cpp
325

OK, that's a possible solution indeed.

329

s/length/count/

337

coding style: missing space after "if"

346

I'll try to keep this in mind when coming back to my KJob-based rewrite of KRun.
I keep postponing it because people keep changing KRun :-)

374

indentation of those last 5 lines seems wrong (2 spaces instead of 4?)

This revision now requires changes to proceed.Dec 13 2019, 10:06 PM

For SMB shares I guess what can happen is that the URL doesn't have a username in it, but it still needs a password? IIRC there was no username in "old" samba (W95, W98, XP, ...?).
This might explain what you were saying about people having password problems even with empty userInfo() (which means username and password).
But we can hardly know, just by looking at a URL, if a password is needed... I guess most of time a password is actually needed? Maybe you're right after all, and we should always send SMB urls to kiofuse..... Dunno.

@ngraham could you confirm if you have the credential issue with the current state of this patch. If you do, I'll simplify the logic seeming as there's no chance I can be "smart" about this here.

feverfew updated this revision to Diff 71543.Dec 14 2019, 6:31 PM
  • fix code style
feverfew retitled this revision from [WIP] Adding support for mounting KIOFuse URLs for applications that don't use KIO to Adding support for mounting KIOFuse URLs for applications that don't use KIO.Dec 14 2019, 6:39 PM
feverfew edited the summary of this revision. (Show Details)
feverfew edited the test plan for this revision. (Show Details)
feverfew updated this revision to Diff 71666.Dec 16 2019, 1:24 PM

fix indentation

I don't have any credentials issues with the current state of the patch and my password-protected Samba share.

@dfaure is this good to go now?

dfaure accepted this revision.Jan 2 2020, 8:44 AM
This revision is now accepted and ready to land.Jan 2 2020, 8:44 AM
This revision was automatically updated to reflect the committed changes.