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

Get app ID via libsystemd for unsandboxed apps #719

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

mwleeds
Copy link
Contributor

@mwleeds mwleeds commented Mar 2, 2022

This is a rework of #581 and dependent on #718, but I'm marking it as a draft because sadly it doesn't work for the one case I actually need it: GNOME Software.

In the case of, say, non-Flatpak Epiphany, the app ID can be successfully gleaned from the systemd unit which looks like app-gnome-org.gnome.Epiphany-182352.scope. But in the case of gnome-software (which runs as /usr/bin/gnome-software --gapplication-service) the unit looks like app-gnome-gnome\x2dsoftware\x2dservice-9502.scope (where 9502 is the pid in my case) so parsing that in any way is not going to get us the correct app ID of org.gnome.Software. So I think we need to look for a better way to get app IDs...

@mwleeds
Copy link
Contributor Author

mwleeds commented Mar 2, 2022

We probably need to do something like what gnome-shell does but the algorithm there is pretty complicated: https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/main/src/shell-window-tracker.c#L380-468

@mwleeds
Copy link
Contributor Author

mwleeds commented Mar 2, 2022

Some notes from @refi64 on this topic:

Okay so the gist of it for non-Flatpak apps was:

  • check if an app ID was set via Wayland protocols (not sure if external clients can check this)
  • check for some GTK-specitic X properties
  • check cgroups I think? (there's something in the code about checking known applications but I couldn't figure it out)
  • check via the values passed to the fd.o startup notification protocol if it was used
  • there's another check I didn't understand involving what I think was called a MetaWindowGroup? Probably something internal to mutter?

Now I think I recall later finding some edge cases that stemmed from some Qt apps using different desktop file names but somehow still being associated by the compositor

Looking through some other comments at the time I had made, there were some other quirks too:

  • some Wayland apps never set an app ID and thus couldn't be matched to anything, my solution at the time was using the executable basename but I'm not sure what Shell does
  • any app IDs need to also be matched against the StartupWMClass values even if the desktop file name doesn't match
  • And on Xorg you're supposed to check WM_CLASS before the GTK specific props

@swick
Copy link
Contributor

swick commented Mar 2, 2022

To me this looks like an issue with gnome-software. The file it installs in /etc/xdg/autostart is called gnome-software-service.desktop which is exactly what systemd then uses as the appid. Renaming it to org.gnome.Software.desktop should fix it.

The question I have however is why this is relevant. Shouldn't the portal be able to work with an appid of gnome\x2dsoftware\x2dservice?

@carlocastoldi
Copy link
Contributor

carlocastoldi commented Mar 2, 2022

To me this looks like an issue with gnome-software. The file it installs in /etc/xdg/autostart is called gnome-software-service.desktop which is exactly what systemd then uses as the appid. Renaming it to org.gnome.Software.desktop should fix it.

I feel like that is the situation as well. Gnome Software doesn't seem to follow the XDG standardization for application. Either because of the .desktop file name or, perhaps, it doesn't set the application_id properly? (doubt)

The question I have however is why this is relevant. Shouldn't the portal be able to work with an appid of gnome\x2dsoftware\x2dservice?

yes, indeed. (?) Since we are talking about an un-sandboxed app it doesn't seem to be that relevant...

EDIT: wait, browsing Gnome Software repo i see they expect 3 .desktop files:

  • org.gnome.Software.desktop (in /usr/share/applications/, for example)
  • gnome-software-local-file.desktop (in /usr/share/applications/, for example)
  • gnome-software-service.desktop (in `/etc/xdg/autostart/, for example)

could it be that the autostarted service is not an actual GUI application? Should it really be associated with the AppID org.gnome.Software?

@mwleeds
Copy link
Contributor Author

mwleeds commented Mar 2, 2022

To me this looks like an issue with gnome-software. The file it installs in /etc/xdg/autostart is called gnome-software-service.desktop which is exactly what systemd then uses as the appid. Renaming it to org.gnome.Software.desktop should fix it.

I feel like that is the situation as well. Gnome Software doesn't seem to follow the XDG standardization for application. Either because of the .desktop file name or, perhaps, it doesn't set the application_id properly? (doubt)

The question I have however is why this is relevant. Shouldn't the portal be able to work with an appid of gnome\x2dsoftware\x2dservice?

yes, indeed. (?) Since we are talking about an un-sandboxed app it doesn't seem to be that relevant...

EDIT: wait, browsing Gnome Software repo i see they expect 3 .desktop files:

  • org.gnome.Software.desktop (in /usr/share/applications/, for example)
  • gnome-software-local-file.desktop (in /usr/share/applications/, for example)
  • gnome-software-service.desktop (in `/etc/xdg/autostart/, for example)

could it be that the autostarted service is not an actual GUI application? Should it really be associated with the AppID org.gnome.Software?

Yes, gnome-software-service.desktop is named that way since it is distinct from org.gnome.Software.desktop. The former has Exec=/usr/bin/gnome-software --gapplication-service and the latter Exec=gnome-software %U. But the gnome-software --gapplication-service process that gets started by gnome-session is the graphical application, it just starts out running in the background and you only use it as a GUI if you launch it from gnome-shell or it is otherwise launched without --gapplication-service.

The reason we need the correct app ID, org.gnome.Software not gnome-software-service, is that we need to be able to check the app ID against an allowlist in the dynamic launcher portal implementation. We also need it to be correct because if we use gnome-software-service as the app ID but then the user later disables the autostart launcher and launches Software from the shell, the app ID would be read as org.gnome.Software and the stored permissions wouldn't be properly granted any more. Similarly for any other reason that we might later get the right app ID, such as improving the app ID algorithm in xdg-desktop-portal.

@carlocastoldi
Copy link
Contributor

Mmh I see. Just one more question I can't answer since at the moment I'm not on Gnome: I don't fully understand what happens when gnome-session starts GS's service in the background and then the user launches GS from gnome-shell without the parameter. Is the systemd slice changed or is it the same of the process launched in background?

In case that it changes, shouldn't the background service be identified as a background.slice?
If it doesn't, do you think it's possible to change the name of the systemd unit only once the GUI app is actually launched?

@mwleeds
Copy link
Contributor Author

mwleeds commented Mar 2, 2022

Mmh I see. Just one more question I can't answer since at the moment I'm not on Gnome: I don't fully understand what happens when gnome-session starts GS's service in the background and then the user launches GS from gnome-shell without the parameter. Is the systemd slice changed or is it the same of the process launched in background?

In case that it changes, shouldn't the background service be identified as a background.slice?

When you launch gnome-software, from the shell or otherwise, the process you launched finds that there's a running instance and exits, and the running instance gets focused. Both before and after you launch Software the running process is in e.g. /user.slice/user-1000.slice/[email protected]/app.slice/app-gnome-gnome\x2dsoftware\x2dservice-13765.scope so it's never in background.slice.

If it doesn't, do you think it's possible to change the name of the systemd unit only once the GUI app is actually launched?

I don't know of any way to make that happen, but even if we could, we want the correct app ID even when Software is running in the background. Portal operations are generally in response to user interaction but it's not inconceivable we would need to use x-d-p in the background, and certainly we would at least connect to the D-Bus service even if we didn't call any methods.

@swick
Copy link
Contributor

swick commented Mar 2, 2022

The reason we need the correct app ID, org.gnome.Software not gnome-software-service, is that we need to be able to check the app ID against an allowlist in the dynamic launcher portal implementation.

Urgh, yeah, so this is not a problem in principal just a weird whitelist case.

We also need it to be correct because if we use gnome-software-service as the app ID but then the user later disables the autostart launcher and launches Software from the shell, the app ID would be read as org.gnome.Software and the stored permissions wouldn't be properly granted any more.

Not in this case, if you start gnome-software from shell it it will find the running instance, tell it to open a window and then exit.

Yes, gnome-software-service.desktop is named that way since it is distinct from org.gnome.Software.desktop

Was this actually a conscious decision? They are after all the same application, the autostart desktop file just starts it in the background.

@mwleeds
Copy link
Contributor Author

mwleeds commented Mar 2, 2022

I proposed a rename of the autostart desktop file: https://gitlab.gnome.org/GNOME/gnome-software/-/merge_requests/1261

It looks like gnome-software-service.desktop used to start a binary called gnome-software-service so that is why it was named as such.

@mwleeds
Copy link
Contributor Author

mwleeds commented Mar 2, 2022

We also need it to be correct because if we use gnome-software-service as the app ID but then the user later disables the autostart launcher and launches Software from the shell, the app ID would be read as org.gnome.Software and the stored permissions wouldn't be properly granted any more.

Not in this case, if you start gnome-software from shell it it will find the running instance, tell it to open a window and then exit.

The hypothetical scenario I mentioned was that the autostart launcher was disabled by the user, and in that case there would be no running instance to find.

gnomesysadmins pushed a commit to GNOME/gnome-software that referenced this pull request Mar 2, 2022
Currently we install three .desktop files:
- org.gnome.Software.desktop which is the one used for the shell's list
  of applications
- gnome-software-local-file.desktop which is used for opening local
  files
- gnome-software-service.desktop which is put in /etc/xdg/autostart/ so
  that "gnome-software --gapplication-service" is run in the background
  at the start of a user session.

This commit renames the autostart one to org.gnome.Software.desktop.
This change is necessary so that the systemd scope created for
gnome-software will follow the standard laid out by
https://systemd.io/DESKTOP_ENVIRONMENTS/ which says that the
Application ID should be in the unit name. With this change the scope
goes from e.g. "app-gnome-gnome\x2dsoftware\x2dservice-9502.scope" to
e.g. "app-gnome-org.gnome.Software-2248.scope". Both before and after
this commit, the scope and process are the same for the background
service and for the graphical application when/if it is launched.

Having the scope (cgroup) in the right format is important because I am
working on getting xdg-desktop-portal to use the cgroup of a process to
determine its app ID[1], and the app ID is then used for checking
permissions, including in the dynamic launcher portal which will
specifically allow the app ID "org.gnome.Software" for one of its
methods.[2]

An interesting historical note: gnome-software-service.desktop used to
have "Exec=${libexecdir}/gnome-software-service" before
"--gapplication-service" came along, so it used to make more sense that
it was named as such.

I have tested this change; starting gnome-software in the background and
via gnome-shell work as expected.

[1] flatpak/xdg-desktop-portal#719
[2] flatpak/xdg-desktop-portal#696
@mwleeds mwleeds force-pushed the mwleeds/app-id-via-systemd branch from 800e7e7 to 8a11540 Compare March 2, 2022 19:50
@mwleeds mwleeds marked this pull request as ready for review March 2, 2022 19:51
@mwleeds mwleeds force-pushed the mwleeds/app-id-via-systemd branch 2 times, most recently from 126e302 to 329d8bf Compare March 2, 2022 20:32
@mwleeds
Copy link
Contributor Author

mwleeds commented Mar 2, 2022

I proposed a rename of the autostart desktop file: https://gitlab.gnome.org/GNOME/gnome-software/-/merge_requests/1261

Even with that change the cgroup does not contain the app ID when the app is launched from the command line: https://gitlab.gnome.org/GNOME/gnome-software/-/merge_requests/1261#note_1398950

I don't think that's necessarily a blocker for merging this but it makes development hard.

@carlocastoldi
Copy link
Contributor

carlocastoldi commented Mar 2, 2022

Even with that change the cgroup does not contain the app ID when the app is launched from the command line

yes, the systemd specification is tailored for desktop environments. If you want to launch a program from terminal such that it complies with the xdg systemd & DE specification you have to do it as described in this snippet: https://invent.kde.org/-/snippets/1111
(spoiler: with systemd-run or xdg-open).

I'd say it's still worth it to push the MR on Gnome Software

Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a couple of nits, but it generally LGTM now. Feel free to merge after addressing them.

src/xdp-utils.c Outdated Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
src/sd-escape.c Show resolved Hide resolved
src/xdp-utils.c Outdated Show resolved Hide resolved
@mwleeds mwleeds force-pushed the mwleeds/app-id-via-systemd branch from e5c886f to 79d13d3 Compare March 7, 2022 22:55
@mwleeds
Copy link
Contributor Author

mwleeds commented Mar 8, 2022

(force-pushed to rebase on main)

src/xdp-utils.c Outdated Show resolved Hide resolved
src/xdp-utils.c Show resolved Hide resolved
@mwleeds mwleeds force-pushed the mwleeds/app-id-via-systemd branch from 79d13d3 to 0942cb0 Compare March 8, 2022 17:27
Copy link
Member

@GeorgesStavracas GeorgesStavracas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR continues to LGTM, the tests are chef's kiss. @hadess should we land this? This is blocking further portals.

@GeorgesStavracas
Copy link
Member

For the sake of progress, given that @mwleeds added corresponding unit tests, I'll take the liberty to go ahead and land this.

@hadess if you have other concerns about this PR, please open an issue for us to discuss there.

This commit adds an optional dependency on libsystemd and, when
available, uses it to attempt to find an application ID even for
processes which are not Snap or Flatpak sandboxes. This is not always
successful, so we still need to handle the case of an empty app ID for
an XdpAppInfo.

Since this is not a perfect solution for finding the app ID, we may
eventually want to introduce a way for *unsandboxed* processes to
specify their own app ID, perhaps a portal-wide SetAppID() method. This
is analogous to the security model gnome-shell has: if a process is
sandboxed use that app ID, and otherwise use information that is under
the control of the application (e.g. WM_CLASS, GApplication ID, etc.).

Helps: #579

(Originally authored by Carlo Castoldi, re-done by Phaedrus Leeds)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants