Bug 97776

Created attachment 130501 [details] [review]
client-info: Replace desktop ID detection

For newer (>= 0.6.10) versions of Flatpak, the way to export the desktop
ID has changed from requiring cgroups to not requiring it.

See https://github.com/flatpak/flatpak/releases/tag/0.6.10

Created attachment 130502 [details] [review]
client-info: Replace desktop ID detection

For newer (>= 0.6.10) versions of Flatpak, the way to export the desktop
ID has changed from requiring cgroups to not requiring it.

See https://github.com/flatpak/flatpak/releases/tag/0.6.10

This changes the private gclue_client_info_get_xdg_id() API in that a
NULL xdg_id should be considered to be disqualifying:
"
Like parse_app_info_from_fileinfo(), returns NULL on failure,
"" if not sandboxed, and a desktop ID otherwise
"

This ensures that sandboxed applications can't change their own
desktop ID through any means.

Comment on attachment 130502 [details] [review]
client-info: Replace desktop ID detection

Review of attachment 130502 [details] [review]:
-----------------------------------------------------------------

::: src/gclue-service-client.c
@@ +428,5 @@
>                  return TRUE;
>          }
>  
> +        if (*desktop_id == '\0') {
> +                /* Non-flatpak app */

So we are going back to require system apps to specify DesktopId? I hate that we need to keep going back and forth on this. :( System apps can lie about desktop ID so it doesn't make much sense to require it from them.

(In reply to Zeeshan Ali from comment #3)
> Comment on attachment 130502 [details] [review] [review]
> client-info: Replace desktop ID detection
> 
> Review of attachment 130502 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: src/gclue-service-client.c
> @@ +428,5 @@
> >                  return TRUE;
> >          }
> >  
> > +        if (*desktop_id == '\0') {
> > +                /* Non-flatpak app */
> 
> So we are going back to require system apps to specify DesktopId? I hate
> that we need to keep going back and forth on this. :( System apps can lie
> about desktop ID so it doesn't make much sense to require it from them.

System apps can also lie about their identifiers when using notifications, doesn't mean that we don't want to see them in the Notifications panel to be able to block them.

We usually prefer "system" notifications to be associated with a graphical application, for example, battery notifications "come from" the Power Settings panel, even if they are implemented somewhere else (in gnome-settings-daemon).

I do think that "night light" should be associated with the Displays panel, the shell's weather should be associated with gnome-weather, etc. Using those identifiers pretty much is a requirement if we want to have any hope of differentiating the users.

Finally, this requires non-Flatpak'ed apps to behave in a way that's at least a little similar to their Flatpak'ed counterparts.

In any case, could you please divide your patch a bit more so we have our new behaviour change in one patch and update for Flatpak change in another?

Comment on attachment 130502 [details] [review]
client-info: Replace desktop ID detection

Review of attachment 130502 [details] [review]:
-----------------------------------------------------------------

::: src/gclue-service-client.c
@@ +423,5 @@
>          if (desktop_id == NULL) {
>                  g_dbus_method_invocation_return_error_literal (invocation,
>                                                                 G_DBUS_ERROR,
>                                                                 G_DBUS_ERROR_ACCESS_DENIED,
> +                                                               "Failed to read Flatpak application information");

Don't think this is a good message. Way too vague and not exactly correct after your change of requiring desktop id from all apps.

@@ +428,4 @@
>                  return TRUE;
>          }
>  
> +        if (*desktop_id == '\0') {

Why check for '\0', when you can just check for NULL and be consistent with rest of code?

(In reply to Zeeshan Ali from comment #6)
> Comment on attachment 130502 [details] [review] [review]
> client-info: Replace desktop ID detection
> 
> Review of attachment 130502 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: src/gclue-service-client.c
> @@ +423,5 @@
> >          if (desktop_id == NULL) {
> >                  g_dbus_method_invocation_return_error_literal (invocation,
> >                                                                 G_DBUS_ERROR,
> >                                                                 G_DBUS_ERROR_ACCESS_DENIED,
> > +                                                               "Failed to read Flatpak application information");
> 
> Don't think this is a good message. Way too vague and not exactly correct
> after your change of requiring desktop id from all apps.

All flatpak'ed apps, not all apps.

> @@ +428,4 @@
> >                  return TRUE;
> >          }
> >  
> > +        if (*desktop_id == '\0') {
> 
> Why check for '\0', when you can just check for NULL and be consistent with
> rest of code?

Because of this:
+/* Like parse_app_info_from_fileinfo(), returns NULL on failure,
+ * "" if not sandboxed, and a desktop ID otherwise */

Let me know if you want the internal API to be different. I'd like to keep the parse_app_info_from_fileinfo() changes to a minimum.

Once we've agreed on that, I can split off the updated checking from the requirement to have a desktop ID.

(In reply to Bastien Nocera from comment #7)
> (In reply to Zeeshan Ali from comment #6)
> > Comment on attachment 130502 [details] [review] [review] [review]
> > client-info: Replace desktop ID detection
> > 
> > Review of attachment 130502 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: src/gclue-service-client.c
> > @@ +423,5 @@
> > >          if (desktop_id == NULL) {
> > >                  g_dbus_method_invocation_return_error_literal (invocation,
> > >                                                                 G_DBUS_ERROR,
> > >                                                                 G_DBUS_ERROR_ACCESS_DENIED,
> > > +                                                               "Failed to read Flatpak application information");
> > 
> > Don't think this is a good message. Way too vague and not exactly correct
> > after your change of requiring desktop id from all apps.
> 
> All flatpak'ed apps, not all apps.

Really? It's checking the return value of gclue_client_info_get_xdg_id(), which is the same for both now with these changes.
> 
> > @@ +428,4 @@
> > >                  return TRUE;
> > >          }
> > >  
> > > +        if (*desktop_id == '\0') {
> > 
> > Why check for '\0', when you can just check for NULL and be consistent with
> > rest of code?
> 
> Because of this:
> +/* Like parse_app_info_from_fileinfo(), returns NULL on failure,
> + * "" if not sandboxed, and a desktop ID otherwise */
> 
> Let me know if you want the internal API to be different. I'd like to keep
> the parse_app_info_from_fileinfo() changes to a minimum.

Right, I missed that.
 
> Once we've agreed on that, I can split off the updated checking from the
> requirement to have a desktop ID.

Sure. agreed on the last part al least. :)

(In reply to Zeeshan Ali from comment #8)
> (In reply to Bastien Nocera from comment #7)
> > (In reply to Zeeshan Ali from comment #6)
> > > Comment on attachment 130502 [details] [review] [review] [review] [review]
> > > client-info: Replace desktop ID detection
> > > 
> > > Review of attachment 130502 [details] [review] [review] [review] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: src/gclue-service-client.c
> > > @@ +423,5 @@
> > > >          if (desktop_id == NULL) {
> > > >                  g_dbus_method_invocation_return_error_literal (invocation,
> > > >                                                                 G_DBUS_ERROR,
> > > >                                                                 G_DBUS_ERROR_ACCESS_DENIED,
> > > > +                                                               "Failed to read Flatpak application information");
> > > 
> > > Don't think this is a good message. Way too vague and not exactly correct
> > > after your change of requiring desktop id from all apps.
> > 
> > All flatpak'ed apps, not all apps.
> 
> Really? It's checking the return value of gclue_client_info_get_xdg_id(),
> which is the same for both now with these changes.

This error only happens if we fail to read the Flatpak app's information. It will return an empty string if the application isn't Flatpak'ed. So the error matches what the failure is.

The error if the DesktopId isn't set on a non-Flatpak'ed app is the same as it was before:
"'DesktopId' property must be set");

Created attachment 139273 [details] [review]
client-info: Replace desktop ID detection for new Flatpak

For newer (>= 0.6.10) versions of Flatpak, the way to export the desktop
ID has changed from requiring cgroups to not requiring it.

See https://github.com/flatpak/flatpak/releases/tag/0.6.10

This changes the private gclue_client_info_get_xdg_id() API to return a
NULL xdg_id should the code fail to read the Flatpak ID for a Flatpak'ed
application, and consider it to be disqualifying:
"
Like parse_app_info_from_fileinfo(), returns NULL on failure,
"" (an empty string) if not sandboxed, and a desktop ID otherwise
"

Created attachment 139274 [details] [review]
service-client: Verify 'DesktopId' against Flatpak ID

And refuse to serve applications that have different values for both.
This makes it easier to detect applications which would lie about their
IDs when run outside Flatpak, as well as making sure that settings are
shared for Flatpak'ed and non-Flatpak'ed versions of the application.

Comment on attachment 139273 [details] [review]
client-info: Replace desktop ID detection for new Flatpak

Review of attachment 139273 [details] [review]:
-----------------------------------------------------------------

OK.

(In reply to Bastien Nocera from comment #11)
> Created attachment 139274 [details] [review] [review]
> service-client: Verify 'DesktopId' against Flatpak ID
> 
> And refuse to serve applications that have different values for both.
> This makes it easier to detect applications which would lie about their
> IDs when run outside Flatpak, as well as making sure that settings are
> shared for Flatpak'ed and non-Flatpak'ed versions of the application.

Any opinions on this one?

(In reply to Bastien Nocera from comment #13)
> (In reply to Bastien Nocera from comment #11)
> > Created attachment 139274 [details] [review] [review] [review]
> > service-client: Verify 'DesktopId' against Flatpak ID
> > 
> > And refuse to serve applications that have different values for both.
> > This makes it easier to detect applications which would lie about their
> > IDs when run outside Flatpak, as well as making sure that settings are
> > shared for Flatpak'ed and non-Flatpak'ed versions of the application.
> 
> Any opinions on this one?

If the desktop id and Flatpak ID is supposed to be always the same, that's correct then.

Attachment 139273 [details] pushed as a5afe7a - client-info: Replace desktop ID detection for new Flatpak
Attachment 139274 [details] pushed as c8dc5bc - service-client: Verify 'DesktopId' against Flatpak ID

And I'm reopening this as well, because while it works with geoclue running as root, it doesn't with geoclue running as the geoclue user :/

Some debug I added to parse_app_info_from_fileinfo():
Geoclue-Message: 13:17:46.522: couldn't open /proc/1905/root

(In reply to Bastien Nocera from comment #16)
> And I'm reopening this as well, because while it works with geoclue running
> as root, it doesn't with geoclue running as the geoclue user :/
> 
> Some debug I added to parse_app_info_from_fileinfo():
> Geoclue-Message: 13:17:46.522: couldn't open /proc/1905/root

Do you know a solution to this or shall I revert until we find a solution?

(In reply to Bastien Nocera from comment #16)
> And I'm reopening this as well, because while it works with geoclue running
> as root, it doesn't with geoclue running as the geoclue user :/
> 
> Some debug I added to parse_app_info_from_fileinfo():
> Geoclue-Message: 13:17:46.522: couldn't open /proc/1905/root

This will require changes in Flatpak and/or the kernel:
https://github.com/flatpak/flatpak/issues/1644

And even when that's fixed, we will probably still need to make changes here. I would advise reverting those changes for now, and we'll still need "systemd --user" to be available to know whether an application is Flatpak'ed.

Created attachment 139303 [details] [review]
Revert "client-info: Replace desktop ID detection for new Flatpak"

This reverts commit a5afe7a0ee971371423edaca4fdd43b9b7b05a1e,
commit c8dc5bc0318293dbc9007946e92a10dba3a57d54 and commit
defe4a3e9f4bacba44b12e1fe82dd915e49858c2.

The new method of detecting whether an application is a Flatpak is only
available to 1) the user running the Flatpak 2) root. As we advise that
geoclue is run as a normal non-privileged user, revert those commits
while we wait for a solution to be available.

See https://github.com/flatpak/flatpak/issues/1644

Attachment 139303 [details] pushed as 6e575c0 - Revert "client-info: Replace desktop ID detection for new Flatpak"

I'll keep it open then till we have a way to handle this.

(In reply to Zeeshan Ali from comment #20)
> Attachment 139303 [details] pushed as 6e575c0 - Revert "client-info: Replace
> desktop ID detection for new Flatpak"
> 
> I'll keep it open then till we have a way to handle this.

Thanks.

Maybe we could move fetching the DesktopId (and its verification) to the agent? I might look at this in a while, but it'd need changing the order of some checks around, and passing a PID to the agent.

We'll see how the flatpak bug goes.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/geoclue/geoclue/issues/53.