Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DynamicLauncher has problems with desktop id outside sandbox #826

Open
JakobDev opened this issue Jun 29, 2022 · 13 comments
Open

DynamicLauncher has problems with desktop id outside sandbox #826

JakobDev opened this issue Jun 29, 2022 · 13 comments
Labels
needs diagnosis Root cause of the issue needs to be diagnosed needs discussion Needs discussion on how to implement or fix the corresponding task portal: dynamic launcher Dynamic Launcher portal

Comments

@JakobDev
Copy link
Contributor

I have the following code:

from PyQt5.QtDBus import QDBusArgument, QDBusVariant, QDBusConnection, QDBusMessage, QDBusPendingCallWatcher, QDBusPendingReply
from PyQt5.QtCore import QByteArray, QObject, pyqtSlot
from typing import Dict, Any


class DynamicLauncherPortal(QObject):
    def __init__(self) -> None:
        super().__init__()
        self._connection = QDBusConnection.sessionBus()

    @pyqtSlot(QDBusMessage)
    def _install_launcher(self, msg: QDBusMessage) -> None:
        if msg.arguments()[0] != 0:
            return

        install_message = QDBusMessage.createMethodCall("org.freedesktop.portal.Desktop", "/org/freedesktop/portal/desktop", "org.freedesktop.portal.DynamicLauncher", "Install")
        install_message.setArguments([msg.arguments()[1]["token"], self._desktop_id, self._desktop_file, {}])
        result = self._connection.call(install_message)
        print(result.errorMessage())

    def create_launcher(self, name: str, icon: bytes, options: Dict[str, Any], desktop_id: str, desktop_file: str) -> bool:
        self._desktop_id = desktop_id
        self._desktop_file = desktop_file

        arg = QDBusArgument()
        arg.beginStructure()
        arg.add("bytes")
        arg.add(QDBusVariant(QByteArray(icon)))
        arg.endStructure()

        message = QDBusMessage.createMethodCall("org.freedesktop.portal.Desktop", "/org/freedesktop/portal/desktop", "org.freedesktop.portal.DynamicLauncher", "PrepareInstall")
        message.setArguments(["", name, QDBusVariant(arg), options])
        result = self._connection.call(message)

        if result.errorName() == "org.freedesktop.DBus.Error.UnknownMethod":
            return False

        self._connection.connect("org.freedesktop.portal.Desktop", result.arguments()[0], "org.freedesktop.portal.Request", "Response", self._install_launcher)

        return True


def main():
    from PyQt5.QtWidgets import QWidget, QApplication
    app = QApplication([])

    app.setDesktopFileName("com.example.test")
    app.setApplicationName("com.example.test")

    w = QWidget()
    w.show()

    with open("test.png", "rb") as f:
        img_data = f.read()

    with open("test.desktop") as f:
        desk = f.read()

    dy = DynamicLauncherPortal()
    dy.create_launcher("Portaltest", img_data, {}, "com.example.test.launcher.desktop", desk)

    app.exec()


main()

As you can see, it sets the Application name to com.example.test:

app.setDesktopFileName("com.example.test")
app.setApplicationName("com.example.test")

It calls the Install method with com.example.test.launcher.desktop as desktop_file_id. If I run it outside the Sandbox, it fails with this error:

Desktop file id missing app id prefix 'org.kde.konsole.': com.example.test.launcher.desktop

As you may see, I run the code through Konsole. If I set the desktop_file_id to org.kde.konsolelauncher.desktop it works. This is really bad. The code should work inside and outside the Sandbox. Inside the Sandbox, the name if always clear: I's the name of the Flatpak. But outside the Sandbox the name depends on how you run the program which is something I don't have control over as a developer.

There are 2 ways to solve this Issue:

  1. Allow Apps outside a Sandbox setting a Appname
  2. Allow Apps outside the Sanbox using a desktop_file_id with any prefix
@mcatanzaro
Copy link
Contributor

CC @mwleeds

@mwleeds
Copy link
Contributor

mwleeds commented Oct 2, 2022

xdg-desktop-portal is determining the app ID for unsandboxed processes by looking at the systemd unit name and interpreting it according to this standard. As discussed here, launching an app from the command line in such a way that it complies with the aforementioned standard is possible but not intuitive. There's another relevant comment here:

Yeah, I don't think we need to cover this use-case for now. Properly spawning applications require some help from the compositor and the session manager these days, and that's primarily what we're trying to improve here.

Also relevant are this and the following comments on the PR that implemented the dynamic launcher portal: #696 (comment)

@mwleeds
Copy link
Contributor

mwleeds commented Oct 2, 2022

As you can see, it sets the Application name to com.example.test:
app.setDesktopFileName("com.example.test")
app.setApplicationName("com.example.test")
It calls the Install method with com.example.test.launcher.desktop as desktop_file_id

But then the portal method is called before that app object has exec() called on it, so you are effectively calling the portal from your script not from the app, unless I'm misunderstanding something? If this code is just a minimal reproducer, what is the actual use case you have that's running into this problem?

@mwleeds mwleeds added the portal: dynamic launcher Dynamic Launcher portal label Oct 7, 2022
@mcatanzaro
Copy link
Contributor

So I'm a little uncertain where this leaves us for Epiphany. It seems we need to either (a) change Ephy somehow to not use the dynamic launcher portal unless we are sandboxed, or (b) switch back to the GNOME 42 version of the code?

@mwleeds
Copy link
Contributor

mwleeds commented Oct 10, 2022

So I'm a little uncertain where this leaves us for Epiphany. It seems we need to either (a) change Ephy somehow to not use the dynamic launcher portal unless we are sandboxed, or (b) switch back to the GNOME 42 version of the code?

I don't understand how this issue affects Epiphany. When Epiphany is run from the desktop launcher (gnome-shell) it should be compliant with the systemd standard, and the portal should have no problem determining its app ID correctly.

@mcatanzaro
Copy link
Contributor

But you cannot assume that Epiphany is run from gnome-shell since it can be run from arbitrary desktops. What about Pantheon and Phosh, which are the desktops that are actually used by the distros that are shipping Epiphany as default browser?

@mwleeds
Copy link
Contributor

mwleeds commented Oct 10, 2022

I didn't mean to imply it had to be gnome-shell. Any desktop environment should be able to comply with that systemd standard when it launches apps, and as far as I know they all do. KDE Plasma definitely does.

@mcatanzaro
Copy link
Contributor

To be clear the problem here is https://gitlab.gnome.org/GNOME/epiphany/-/issues/1859 and it results in Epiphany web apps crashing.

@swick
Copy link
Contributor

swick commented Oct 10, 2022

I really think we should just bite the bullet and implement a setAppId that in xdg-desktop-portal. Yeah, it can be abused if it's not sandboxed but so can everything else.

@mwleeds
Copy link
Contributor

mwleeds commented Oct 10, 2022

I'm not opposed to making a SetAppID method available to unsandboxed processes, as long as we do some research on the implications of that first. But it seems to me the root cause of https://gitlab.gnome.org/GNOME/epiphany/-/issues/1859 is not fully understood and it's not as simple as being "this issue".

@JakobDev
Copy link
Contributor Author

I think the best way would be to let unsandboxed App use every ID they like

@GeorgesStavracas
Copy link
Member

I think the best way would be to let unsandboxed App use every ID they like

Very strong disagree here. This should not ever be allowed by xdg-desktop-portal.

@swick
Copy link
Contributor

swick commented Jan 3, 2023

Yeah, the information of the SetAppID method should only be used if automatic detection failed.

@GeorgesStavracas GeorgesStavracas added needs diagnosis Root cause of the issue needs to be diagnosed needs discussion Needs discussion on how to implement or fix the corresponding task labels Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs diagnosis Root cause of the issue needs to be diagnosed needs discussion Needs discussion on how to implement or fix the corresponding task portal: dynamic launcher Dynamic Launcher portal
Projects
Status: Triaged
Development

No branches or pull requests

5 participants