GNOME Bugzilla – Bug 761245
Per-app geolocation access control
Last modified: 2016-03-03 17:02:31 UTC
I think that it would be really good if in the Privacy section you could specify which applications you want to allow to find your geographical location, because currently you can only turn it off and on globally, but one may want to allow one application to get this information, but not to allow others. So I think that the option to globally turn it on should stay, but there should also be an option where one can select only specific applications to have this enabled for them. I initially filed a report on this issue here but was told to also do so upstream: https://bugs.launchpad.net/ubuntu-gnome/+bug/1513205
Well this feature has been in the plans ever since the design of geoclue2 and we even have D-Bus API to make it possible but unfortunately we currently can not identify apps reliably at all and they can easily pretend to be another app. We're awaiting on App sandboxing for that: https://wiki.gnome.org/Design/Whiteboards/ApplicationSandboxing Alex tells me that he'll soon be able to start implementing sandboxing, now that app bundling is getting in a good production stage.
Actually I had a discussion with Alex here yesterday and turns out that we might already have everything in xdg-app and geoclue to make this work with very little changes in geoclue. We'll only be able to restrict bundled apps running under xdg-app though and will have to trust the apps running on the system. I'm currently setting up xdg-app env to hack on this..
Patches for per-app authorization in gnome-shell already uploaded and hopefully will be in today's release: https://bugzilla.gnome.org/show_bug.cgi?id=762119 Now working on control-center part. Hopefully I'll have it working by tomorrow.
Oh good, and is this going to be backported into GNOME 3.18 or is it only going into 3.19? Or which versions are getting these?
Created attachment 322166 [details] [review] Move get_smart_date() to common utils And rename it to cc_util_get_smart_date(). In a following patch, we'll need to use it from privacy panel.
Created attachment 322167 [details] [review] privacy: Move "Location Services" into a dialog We are about to add per-application settings for geolocation access and they won't really fit well in the main view. This is as per design mockup: https://dl.dropboxusercontent.com/u/5031519/privacy/privacy-3.20.png
Created attachment 322168 [details] [review] privacy: Indicate location service being used As per the new mockup, we should indicate when location service is in use: https://dl.dropboxusercontent.com/u/5031519/privacy/privacy-3.20.png
Created attachment 322169 [details] [review] privacy: Per-app location access control Latest gnome-shell (3.19.91) now asks user if they'd want to allow the application to gain access to their location information when an application tries to access this information. If the user authorizes an application three times in a row, their preference is saved in xdg-app's permission store and user can no longer can change their mind about this later on. Hence the need to provide these per-application controls in control-center.
(In reply to cooks.go.hungry from comment #4) > Oh good, and is this going to be backported into GNOME 3.18 or is it only > going into 3.19? Or which versions are getting these? I wont have time for any backporting at least. It's mainly targetted for xdg-app so not sure it makes much sense in 3.18 anyway.
Created attachment 322195 [details] [review] privacy: Per-app location access control v2: * Disable the applications' UI if geolocation is disabled. * Remove an unused variable.
Forgot to mention that for these patches to work correctly, you'll need the patch from bug#762559 as well.
Review of attachment 322195 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +42,3 @@ +#define APP_PERMISSIONS_TABLE "gnome" +#define APP_PERMISSIONS_ID "geolocation" + Are these permissions registered somewhere to avoid clashes and confusion ? Should this be defined on the permission store side ? @@ +554,3 @@ + { + tmp = value[0]; + value[0] = state ? "EXACT" : "NONE"; Is the format of these entries documented anywhere ? Why uses strings here and not an enum ? @@ +682,3 @@ + g_warning ("%s", error->message); + g_error_free (error); + This should probably be reflected in some way in the UI - show a placeholder where the applications would normally be, or something like that. @@ +788,3 @@ + "org.freedesktop.XdgApp", + "/org/freedesktop/XdgApp/PermissionStore", + "org.freedesktop.XdgApp.PermissionStore", Which module is providing the permission store ? Is this in xdg-app ? This makes that module effectively a dependency of the core (not a problem, but it needs to be communicated and reflected in the modulesets)
Review of attachment 322195 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +42,3 @@ +#define APP_PERMISSIONS_TABLE "gnome" +#define APP_PERMISSIONS_ID "geolocation" + > Are these permissions registered somewhere to avoid clashes and confusion ? Not really. > Should this be defined on the permission store side ? That would be a good question for Alex. @@ +554,3 @@ + { + tmp = value[0]; + value[0] = state ? "EXACT" : "NONE"; They are (can't give you link at the moment due to fdo down). The permissions are stored as strings so easier to just compare strings than define the enum, convert from string to enum and then compare the enums. @@ +682,3 @@ + g_warning ("%s", error->message); + g_error_free (error); + Not sure about that. This is not per-app but rather Lookup call gives you all app permissions for our table/store. This failing most likely either means: * xdg-app is not installed * no app has yet access location and therefore 'gnome' table has not yet been created. @@ +788,3 @@ + "org.freedesktop.XdgApp", + "/org/freedesktop/XdgApp/PermissionStore", + "org.freedesktop.XdgApp.PermissionStore", xdg-app but we don't have a hard-dep at all. If it's not present/installed, users just don't get he new per-app authorization for location data.
Review of attachment 322195 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +554,3 @@ + { + tmp = value[0]; + value[0] = state ? "EXACT" : "NONE"; FDO is back: https://www.freedesktop.org/software/geoclue/docs/geoclue-gclue-enums.html#GClueAccuracyLevel
Review of attachment 322166 [details] [review]: You need to prefix the commit subject, "common: " is probably a good one. ::: panels/common/cc-util.c @@ +123,3 @@ + if (span <= 0) { + label = g_strdup (_("Today")); + } Can you also prepare a separate commit that will fix the coding style to match the rest of the code there? ::: panels/common/cc-util.h @@ +25,3 @@ +char * cc_util_normalize_casefold_and_unaccent (const char *str); +gchar *cc_util_get_smart_date (GDateTime *date); Use char, as the other function does.
Review of attachment 322167 [details] [review]: Looks fine.
Review of attachment 322168 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +404,3 @@ + if (in_use) { + gtk_label_set_label (GTK_LABEL (priv->location_label), _("In use")); + No need for a linefeed here... @@ +410,3 @@ + on = g_settings_get_boolean (priv->location_settings, LOCATION_ENABLED); + gtk_label_set_label (GTK_LABEL (priv->location_label), + on ? _("On") : _("Off")); This will need to use contexts. @@ +441,3 @@ + if (error != NULL) + { + g_warning ("%s", error->message); That's awfully terse and you need to check whether the error is G_IO_ERROR_CANCELLED. @@ +443,3 @@ + g_warning ("%s", error->message); + g_error_free (error); + ...or here. @@ +471,2 @@ + add_row (self, + _("Location Services"), That can probably go all on one row. @@ +483,3 @@ G_SETTINGS_BIND_DEFAULT); + + g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, You need to use a cancellable here, so you can cancel the call when the panel goes away before the proxy is ready.
Review of attachment 322195 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +682,3 @@ + g_warning ("%s", error->message); + g_error_free (error); + We cannot be expecting xdg-app to be installed (yet), or even expecting apps to have been using location services before launching the panel. We need to make sure that a system-provided (and trusted) "org.gnome.Maps" application will use the same configuration as an xdg-app "org.gnome.Maps". You could pre-populate the table using a tag in the .desktop file, as the notifications panel does ("X-GNOME-UsesLocationServices" for example). As in the previous patch, the error message is too terse, and you need to check for cancellation. @@ +726,3 @@ + if (error != NULL) + { + g_warning ("%s", error->message); Terse error message. @@ +740,3 @@ + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, Cancellable. @@ +785,3 @@ + g_dbus_proxy_new_for_bus (G_BUS_TYPE_SESSION, + G_DBUS_PROXY_FLAGS_NONE, + NULL, Needs a cancellable and be cancelled when exiting the panel.
Review of attachment 322168 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +410,3 @@ + on = g_settings_get_boolean (priv->location_settings, LOCATION_ENABLED); + gtk_label_set_label (GTK_LABEL (priv->location_label), + on ? _("On") : _("Off")); contexts?
Review of attachment 322195 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +682,3 @@ + g_warning ("%s", error->message); + g_error_free (error); + > We cannot be expecting xdg-app to be installed (yet) And we're not. We don't show the new UI if we fail to lookup the table in the permission store. > or even expecting apps to have been using location services before launching the panel. That neither. If no app has used location services yet, we get an error here and we don't show the UI. Exactly same happens if table exists but there are no apps to show. > We need to make sure that a system-provided (and trusted) "org.gnome.Maps" application will use the same configuration as an xdg-app "org.gnome.Maps". * I don't think so. if xdg-app is not installed, everything just works the same as in 3.18 for all apps. If xdg-app is installed, good we allow users to control per-app authorizations, just that system-apps could lie about their ID but xdg-apps can't (which is fine, since this is mainly targetted towards xdg-app). * We are already in UI freeze so if you think that is a must, we gotta pull off patches from gnome-shell and postpone this to 3.22.
If this is postponed to 3.22, could it not then be backported into 3.20?
(In reply to Zeeshan Ali (Khattak) from comment #19) > Review of attachment 322168 [details] [review] [review]: > > ::: panels/privacy/cc-privacy-panel.c > @@ +410,3 @@ > + on = g_settings_get_boolean (priv->location_settings, LOCATION_ENABLED); > + gtk_label_set_label (GTK_LABEL (priv->location_label), > + on ? _("On") : _("Off")); > > contexts? as in C_("location status", "On")
(In reply to Zeeshan Ali (Khattak) from comment #20) > * We are already in UI freeze so if you think that is a must, we gotta pull > off patches from gnome-shell and postpone this to 3.22. We can always ask for exceptions. If you want definite answers to your design questions, it would be great to port screenshots of those to make the review quicker. (In reply to cooks.go.hungry from comment #21) > If this is postponed to 3.22, could it not then be backported into 3.20? No, new features aren't backported, because then they'd miss documentation and translation, which is what the freeze period is supposed to avoid.
So is it definite then that it won't be implemented in 3.20 and rather in 3.22?
(In reply to Bastien Nocera from comment #23) > (In reply to Zeeshan Ali (Khattak) from comment #20) > > * We are already in UI freeze so if you think that is a must, we gotta pull > > off patches from gnome-shell and postpone this to 3.22. > > We can always ask for exceptions. Yes but surely before code freeze and most definitely before 3.22 release. I had a discussion with Alex while you were on vacation and we both thought xdg-app' permission store is the best place for these permissions so I went ahead with that approach. Now I would hate to make a quick decision to change that now to be able to be able to implement the changes in 3.20 time. > If you want definite answers to your > design questions, it would be great to port screenshots of those to make the > review quicker. Screenshots aren't working for me when I'm running self-built gnome-shell from jhbuild (among a few other things) so that'll be difficult. I'm not exactly sure though why you need a screenshot to decide where app permissions should be stored.
(In reply to Matthias Clasen from comment #22) > (In reply to Zeeshan Ali (Khattak) from comment #19) > > Review of attachment 322168 [details] [review] [review] [review]: > > > > ::: panels/privacy/cc-privacy-panel.c > > @@ +410,3 @@ > > + on = g_settings_get_boolean (priv->location_settings, LOCATION_ENABLED); > > + gtk_label_set_label (GTK_LABEL (priv->location_label), > > + on ? _("On") : _("Off")); > > > > contexts? > > as in > > C_("location status", "On") Ah that. I'm not actually introducing a new string here though, just moving it so i'll provide a separate patch for that.
Review of attachment 322168 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +471,2 @@ + add_row (self, + _("Location Services"), Nope, unless the coding-style is 120 chars line rather than 80.
Created attachment 322486 [details] [review] common: Fix codying-style in cc_util_get_smart_date()
Created attachment 322487 [details] [review] privacy: Indicate location service being used As per the new mockup, we should indicate when location service is in use: https://dl.dropboxusercontent.com/u/5031519/privacy/privacy-3.20.png
Created attachment 322488 [details] [review] privacy: Per-app location access control Latest gnome-shell (3.19.91) now asks user if they'd want to allow the application to gain access to their location information when an application tries to access this information. The user's choice is saved in xdg-app's permission store and user can no longer can change their mind about this later on. Hence the need to provide these per-application controls in control-center. --- This patch assumes the patch in bug#762559 to have been merged.
Created attachment 322522 [details] Screenshot of main privacy panel
Created attachment 322523 [details] Screenshot of location services dialog
Created attachment 322524 [details] Screenshot of location services dialog (geolocation disabled)
Created attachment 322525 [details] Screenshot of location services dialog (No xdg-app installed)
Review of attachment 322486 [details] [review]: Sure.
Review of attachment 322487 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +407,3 @@ + } + + if (in_use) { Brace on the next line here. @@ +413,3 @@ + + on = g_settings_get_boolean (priv->location_settings, LOCATION_ENABLED); + label = on ? C_(LOCATION_STATUS_CONTEXT, "On") : I'm not actually sure that this will work as expected. Can you please double-check that: make gnome-control-center-2.0.pot in the po/ directory will extract the string properly? @@ +423,3 @@ + gpointer user_data) +{ + update_location_label (CC_PRIVACY_PANEL (user_data)); There's no need to cast the user_data. update_location_label (user_data); shouldn't warn. @@ +432,3 @@ + gpointer user_data) +{ + update_location_label (CC_PRIVACY_PANEL (user_data)); Ditto. @@ +440,3 @@ + gpointer user_data) +{ + CcPrivacyPanel *self = CC_PRIVACY_PANEL (user_data); Ditto, and see below why. @@ +444,3 @@ + + self->priv->gclue_manager = g_dbus_proxy_new_for_bus_finish (res, &error); + if (error != NULL) This is wrong. 1) you can't access self before checking whether the call was cancelled, because if it was, for example because ->finalize was called, then the pointer is dangling. This should be: proxy = g_dbus_...finish (res, &error); if (!proxy) { ... } self->priv->gclue_manager = proxy;
Review of attachment 322488 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +514,3 @@ + data->changing_state = FALSE; + + results = g_dbus_proxy_call_finish (self->priv->perm_store, res, &error); Can't access the user_data here, again. @@ +606,3 @@ + LocationAppStateData *data; + + desktop_id = g_strjoin (".", app_id, "desktop", NULL); isn't: g_strdup_printf ("%s.desktop", app_id); easier? You're leaking the desktop_id as well. @@ +702,3 @@ + gint64 last_used; + + if (g_strv_length (value) < 2) Add some g_debug() here, to explain why you're skipping the entry. @@ +730,3 @@ + GError *error = NULL; + + self->priv->perm_store = g_dbus_proxy_new_for_bus_finish (res, &error); Same as in the previous patch. This pattern will cause crashes when exiting the panel if the proxy creation is cancelled.
Review of attachment 322487 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +413,3 @@ + + on = g_settings_get_boolean (priv->location_settings, LOCATION_ENABLED); + label = on ? C_(LOCATION_STATUS_CONTEXT, "On") : Actually, it doesn't but it does if I hardcode the context string here.
Created attachment 322794 [details] [review] privacy: Indicate location service being used As per the new mockup, we should indicate when location service is in use: https://dl.dropboxusercontent.com/u/5031519/privacy/privacy-3.20.png
Created attachment 322795 [details] [review] privacy: Per-app location access control Latest gnome-shell (3.19.91) now asks user if they'd want to allow the application to gain access to their location information when an application tries to access this information. The user's choice is saved in xdg-app's permission store and user can no longer can change their mind about this later on. Hence the need to provide these per-application controls in control-center.
Review of attachment 322794 [details] [review]: > https://dl.dropboxusercontent.com/u/5031519/privacy/privacy-3.20.png Please find a URL that will be a little bit more permanent (should be in the gnome-design github, or on the wiki) Please also file a new bug about handling geoclue exiting after a period of inactivity, because the current code here won't. Looks fine otherwise. ::: panels/privacy/cc-privacy-panel.c @@ +444,3 @@ + + proxy = g_dbus_proxy_new_for_bus_finish (res, &error); + if (error != NULL) if (proxy == NULL)
Review of attachment 322795 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +513,3 @@ + GError *error = NULL; + + data->changing_state = FALSE; Again, data will be freed memory on finalize, when that call gets cancelled, and the widget is destroyed. @@ +542,3 @@ + GVariantBuilder builder; + + if (data->changing_state) Ditto. @@ +684,3 @@ + GError *error = NULL; + + priv->location_apps_perms = g_dbus_proxy_call_finish (priv->perm_store, Still a no.
Review of attachment 322794 [details] [review]: > Please also file a new bug about handling geoclue exiting after a period of inactivity, because the current code here won't. Hm.. does it matter with this usage? We only need to know if geoclue is being used and it's certainly not being used if it's not running.
(In reply to Zeeshan Ali (Khattak) from comment #43) > Review of attachment 322794 [details] [review] [review]: > > > Please also file a new bug about handling geoclue exiting after a period of inactivity, because the current code here won't. > > Hm.. does it matter with this usage? We only need to know if geoclue is > being used and it's certainly not being used if it's not running. I'm pretty sure that it will autostart the daemon, and listen to it while it's there. But when it disappears, and then reappears, I'm not sure that the proxy would still be connected to the backend service. If you test it and it works as expected, then feel free to ignore. I'm fairly certain it won't work, but there's just one way of checking that :)
(In reply to Bastien Nocera from comment #44) > (In reply to Zeeshan Ali (Khattak) from comment #43) > > Review of attachment 322794 [details] [review] [review] [review]: > > > > > Please also file a new bug about handling geoclue exiting after a period of inactivity, because the current code here won't. > > > > Hm.. does it matter with this usage? We only need to know if geoclue is > > being used and it's certainly not being used if it's not running. > > I'm pretty sure that it will autostart the daemon, and listen to it while > it's there. But when it disappears, and then reappears, I'm not sure that > the proxy would still be connected to the backend service. > > If you test it and it works as expected, then feel free to ignore. I'm > fairly certain it won't work, but there's just one way of checking that :) I just tested it here a few times and proxy neither keeps geoclue alive, nor does it loose it's 'connection' with geoclue over geoclue going away and coming back. i-e it works exactly like I thought it should. :)
Created attachment 322867 [details] Screencast showing "In use" label working fine over geoclue restarts
Created attachment 322869 [details] [review] privacy: Indicate location service being used https://dl.dropboxusercontent.com/u/5031519/privacy/privacy-3.20.png ---- I'll fix the URL once I get it from aday. :)
Created attachment 322871 [details] [review] privacy: Per-app location access control Latest gnome-shell (3.19.91) now asks user if they'd want to allow the application to gain access to their location information when an application tries to access this information. The user's choice is saved in xdg-app's permission store and user can no longer can change their mind about this later on. Hence the need to provide these per-application controls in control-center.
The mockups now live in the gnome-mockups repository (they haven't been changed): https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/privacy/privacy-3.20.png
Review of attachment 322869 [details] [review]: Looks fine, please update the mockup URL though.
Review of attachment 322871 [details] [review]: Looks good to me.
Attachment 322167 [details] pushed as 3ca5b11 - privacy: Move "Location Services" into a dialog Attachment 322486 [details] pushed as 5ebf3d8 - common: Fix codying-style in cc_util_get_smart_date() Attachment 322869 [details] pushed as d77ec8e - privacy: Indicate location service being used Attachment 322871 [details] pushed as e750932 - privacy: Per-app location access control