Camera: use an empty app id for host apps

[grulja]

Sending an app id that doesn't resolve to valid app info (e.g. a desktop file is not found) will automatically reject camera request, because the backend is not able to verify whether the app (based on app id) runs in the background. Same will happen if the app id resolves to an app where our app was started from. This happens for example when you start apps
in GNOME using Alt + F2, where we get invalid app id from cgroups and since it doesn't find opened application window for this app, it will automatically reject the request. Using an empty id fixes this problem.

[swick]

The current version looks good to me

[grulja]

@jadahl any final word? If no, can you merge it? I seem to have power to do it myself, but probably shouldn't as I'm not a maintainer here :)

[jadahl]

Since this changes policy about how to deal with permissions (even though only for non-sandboxed apps), I think we should let it sit here for a few days. If no objections, then it can land.

[A6GibKm]

Would it make sense to add a app-id option to the options in AccessCamera?

[swick]

If we want to provide a way for apps to specify their app id (only when the app id cannot be inferred) then it probably makes sense to add a new interface just for that to avoid having to add it to every portal.

IOW, I think this is out of scope here.

[jadahl]

Indeed out of scope, right now, the app id is always inferred and never explicitly set by apps.

[A6GibKm]

Would a window handle improve the situation compared to an app-id? Anyways, yeah this is out of scope.

This MR looks good to me, but I missed an explanation of why the empty string fixes the issue.

[jadahl]

Would a window handle improve the situation compared to an app-id? Anyways, yeah this is out of scope.

Window handles are always about window management stacking order, i.e. placing a dialog on top of another window, which isn't needed here.

This MR looks good to me, but I missed an explanation of why the empty string fixes the issue.

It's to handle cases where one e.g. runs some app from a terminal and gets an cgroup that resolves to some bogus "app id". Detecting that and falling back to "" means the portal backend can handle it as "this is an unidentifiable app".

[GeorgesStavracas]

What happens with the set_permission_sync (app_id, ...) call below?

https://github.com/flatpak/xdg-desktop-portal/pull/1512/files#diff-3ca5b7fe0d5c5156df93b05804d4ef171f7db564c50377417500da159ee67ab4R153-R157

[grulja]

What happens with the set_permission_sync (app_id, ...) call below?

https://github.com/flatpak/xdg-desktop-portal/pull/1512/files#diff-3ca5b7fe0d5c5156df93b05804d4ef171f7db564c50377417500da159ee67ab4R153-R157

Not sure what you are thinking of, but it will store the permission with an invalid app-id. In the case I'm trying to fix it will store the permission for firefox so the next time it tries to get access to the camera (assuming FF is started the same way as before) it will not ask for permission. I already talked to Jonas about this before and there is no point trying to get the right app-id for non-sandboxed apps, because apps can easily pretend they are different app. Even starting apps from terminal will end up asking for access as the terminal app instead.

[jadahl]

Not sure what you are thinking of, but it will store the permission with an invalid app-id.

Might make more sense to put something more predictable there. I see 4 (or 5) options:

1. Still pass around the actual app ID, but only if it resolves to a valid app info. If no app info, query with "" and store the response as "" in the permission store.
2. Still pass around the actual app ID, but only if it resolves to a valid app info. If no app info, query with "" but never store anything in the permission store.
3. Always pass around "" as the app ID for host apps, and store the response as "" in the permission store.
4. Always pass around "" as the app ID for host apps, but never store any response in the permission store.

Option 1 means apps launched properly integrate properly, and when they are launched "incorrectly" (from a terminal, or anything else that doesn't create the right cgroup namespace) they will get the "global" permission. A bit inconsistent.

Option 2 means apps launched properly can remember permission, but when launched "incorrectly" they will be nagged every time. Also a bit inconsistent.

Option 3 means all host apps are treated as one single remembered thing, meaning once host Firefox is granted, it won't query later when any other host app asks for camera access.

Option 4 means all host apps get nagged every time no matter what.

What is done now is another thing: store a potentially arbitrary app ID in the permission store (e.g. completely random terminal session "id"), I think we probably should avoid.

Option 3 and 4 results in consistency, which I'd argue makes most sense.

Or perhaps there is a 5th option, which is option 3, but instead of the permission store, remember it for the current session only, meaning once you log in again next time, you'd be queried.

[grulja]

Might make more sense to put something more predictable there. I see 4 (or 5) options:
......

I'm in favor of Option 3, however, in such case it might make more sense to completely skip asking for permissions for host apps. Option 4, being nagged every time, is not going to be very well received by users. Option 2 is also going to be annoying, because people will be wondering why running apps in a different ways result into different behavior.

[A6GibKm]

in such case it might make more sense to completely skip asking for permissions for host apps.

I am not sure this is the best, Yes, a host app could force its way to do whatever, but going through the permission system is a courtesy they can also choose to provide to the users.

[grulja]

in such case it might make more sense to completely skip asking for permissions for host apps.

I am not sure this is the best, Yes, a host app could force its way to do whatever, but going through the permission system is a courtesy they can also choose to provide to the users.

My point is that if we are going to ask just once for whatever host app that uses it first and have the others with granted access, then there is probably no point asking even the first time. This only in case we are going to treat all host apps as "global" and not using app ids, because it's not reliable.

[swick]

What's inconsistent about option 1?

Either way, I feel like trying to make things consistent by implementing any of those policies in all portals is not going to work out well. We need to enforce the policy at a single point in the code.

[Mikenux]

Displaying a permission prompt means (1) that the permission is stored for the app requesting it, and (2) that only the right app can access the camera. If these conditions are not met, it is not a good idea to display a prompt.

Also, what is supposed to be displayed in this prompt? If it results in the wrong app name being displayed, it is not a good idea to display a prompt. There is (was?) this bug when launching an app from GNOME Software (e.g. launching Flameshot to get the screenshot permission prompt).

[grulja]

Do we merge this change as is or do we make a decision how to do things differently? I would really like to get this sorted as soon as possible to fix Firefox and other apps.

[jadahl]

I think the reasonable thing is to land this as is, as we ideally should have a working Firefox using the xdg-desktop-portal 1.18 stable branch. To get host apps get more reliable identifiable is complicated, and unlikely something we can fix in 1.18, and whether to always use "" for all portals globally probably needs further discussion and looking at each portal individually if it makes sense. Having Firefox working during that time would be good.

[jadahl]

I think the reasonable thing is to land this as is

Or rather, I mean option 3 here. I read the current code wrong, the "is host app" condition is always TRUE in the branch it's called in, and the current version makes firefox not getting access to the camera if it happens to be opened within the same cgroup as some other application.

[grulja]

Changed to Option 3 suggested by @jadahl.

[jadahl]

Merging as a stop gap solution until the host app identification situation has improved.

[swick]

The code could have used some comments tbh...

[Mikenux]

Option 3 means having a different permission model. Currently, permissions are tied to sandboxed apps and per app. With option 3, permissions are now tied to host apps and also asked generally. It is possible to have, but it will results in three kind of apps instead of two: host (unsandboxed) apps without portals, host apps with portals, and sandboxed apps (with or without portals). If granting access for all host apps one time, one might not know that some host apps are not using any permission.

[mcatanzaro]

This introduced a regression: Camera permissions not set when snapshot launched from overview rather than from terminal.

[grulja]

This introduced a regression: Camera permissions not set when snapshot launched from overview rather than from terminal.

You are absolutely right in your findings. We missed that the permission is also being queried in OpenPipeWireRemote.

[grulja]

This is my fault. Even though I have been testing this with Firefox, we have a fallback in case of a Camera portal failure and we fallback to V4L2 instead so that's why I missed this.

[mcatanzaro]

Fix: #1572