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

Improve host XdpAppInfo handling and force empty app id for Camera portal #1519

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

swick
Copy link
Contributor

@swick swick commented Nov 27, 2024

  • The first commit adds a missing environment variable to the tests README
  • The next commit tightens the requirements for host app id's detected from the cgroup to also require a matching GAppInfo
  • The third commit cleans up the camera portal function for querying permissions
  • The last commit forces an empty app id for the Camera portal (essentially the same as Camera: use an empty app id for host apps #1512)

@swick swick force-pushed the wip/app-info-camera branch 3 times, most recently from 79eb758 to 831a6c7 Compare December 3, 2024 14:21
@swick
Copy link
Contributor Author

swick commented Dec 3, 2024

Now that #1512 has been merged this PR only contains what I hope to be uncontroversial changes.

@jadahl jadahl self-requested a review December 3, 2024 14:26
@jadahl jadahl enabled auto-merge (rebase) December 10, 2024 14:17
@jadahl
Copy link
Collaborator

jadahl commented Dec 11, 2024

This branch has conflicts that must be resolved

auto-merge was automatically disabled December 11, 2024 13:18

Head branch was pushed to by a user without write access

@swick swick force-pushed the wip/app-info-camera branch from 831a6c7 to 508837e Compare December 11, 2024 13:18
@swick
Copy link
Contributor Author

swick commented Dec 11, 2024

Rebased

if (!gappinfo)
{
g_clear_pointer (&desktop_id, g_free);
desktop_id = g_strdup ("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

desktop_id isn't used after this, so this doesn't actually do anything.

Also this came up on Matrix; it might be useful to pass app ids to the backend in some cases where one doesn't necessarily need a .desktop file, more specifically when the desktop environment launches a service that should use portals in a specially crafted cgroup, that the backend will special case.

The real use case is running Xwayland under a cgroup giving it an app ID, and special casing its use of liboeffis (libei via remote desktop portal).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

desktop_id isn't used after this, so this doesn't actually do anything.

Yikes, that was supposed to be appid. Reworked this and moved the logic to get_appid_from_pid.

The real use case is running Xwayland under a cgroup giving it an app ID, and special casing its use of liboeffis (libei via remote desktop portal).

I'd rather we pass the pidfd to backends which gives them the ability to look up cgroups and other kinds of special handling. Abusing the thing that's supposed to identify apps isn't a great idea imo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Passing pidfds could perhaps be useful, but the thing that is already there is the app ID. Perhaps split that part out, so the rest doesn't need to linger, then we can consider what to do with requiring a desktop file to have host app ID's be a thing separately.

Copy link
Contributor Author

@swick swick Dec 16, 2024

Choose a reason for hiding this comment

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

The app id has never meant the cgroup of a process and just because that kind-of practically is the case right now in some cases doesn't mean that we should keep that behavior or encourage people to build on top of that!

I'm really annoyed that any kind of push towards improving and tightening our app detection is met with resistance. Especially because nothing currently depends on it!

The app id detection is still unreliable and some backends do check if
the window matches the app id. Because that app id for host apps comes
from the app itself through wayland, the checks can fail.

For now we just disable app id detection for host apps for the camera
portal entirely. This should be removed when app id detection has become
more reliable.
This should prevent cases where the connection is from something that's
not actually an app but follows the systemd cgroup standard.

This is safe because
* it's a host app anyway
* we always use an empty app id when x-d-p is built without systemd
* it's the default when the systemd cgroup standard is not followed
@swick swick force-pushed the wip/app-info-camera branch from 508837e to f0ae755 Compare December 16, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

2 participants