|Summary:||Ability to get list of connected clients|
|Product:||GeoClue||Reporter:||David Hewitt <davidmhewitt>|
|Component:||service||Assignee:||Geoclue Bugs <geoclue-bugs>|
|Status:||RESOLVED MOVED||QA Contact:|
|i915 platform:||i915 features:|
Add clients property to manager
Description David Hewitt 2017-09-16 22:25:20 UTC
Created attachment 134283 [details] [review] patch Please bear with me as this is my first time using Bugzilla to submit a patch. While working on an indicator that appears in the panel/tray in elementary OS when applications are using GeoClue to fetch the devices location, one feature that we wish to implement is a list of those applications. In the case above, this allows the user to have some awareness of what applications are accessing their location and I believe this would be useful feature in other desktop environments. This patch enables that functionality by adding a method that returns an array of the desktop IDs that are currently accessing GeoClue. I look forward to hearing your feedback. Thanks, David Hewitt
Comment 1 Zeeshan Ali 2017-09-19 13:22:59 UTC
Thanks. While I understand the use case, there is a privacy issue involved: apps should not have access to this API but only agents. i-e we need to ensure this property is not accessible to anyone but the agents. See gclue_service_client_handle_get_property() and gclue_service_client_handle_set_property() function definitions to see how to implement this.
Comment 2 Zeeshan Ali 2017-09-19 13:32:47 UTC
Comment on attachment 134283 [details] [review] patch Review of attachment 134283 [details] [review]: ----------------------------------------------------------------- Looks very good for a first patch actually. We already have a term for "indicators/prefs app": agent. :) ::: src/gclue-service-manager.c @@ +272,5 @@ > + GClueServiceManager *self = GCLUE_SERVICE_MANAGER (manager); > + GClueServiceManagerPrivate *priv = self->priv; > + GList *clients, *l; > + int i = 0; > + GStrv s = g_new (gchar*, g_hash_table_size (priv->clients) + 1); * No reason for a GStrv if we are going to have to transform it into a flat array afterwards. Just create a flat char * directly. * Please prefer slightly descriptive variable names. @@ +282,5 @@ > + } > + > + g_list_free (clients); > + s[g_hash_table_size (priv->clients)] = NULL; > + gclue_dbus_manager_complete_get_client_list (manager, invocation, (const gchar * const *)s); does this call take ownership of s? If not, it's leaking here and you can just use an array on the stack instead with dynamically allocated members that you can free using g_strfreev(). ::: src/org.freedesktop.GeoClue2.Manager.xml @@ +60,5 @@ > + <!-- > + GetClientList: > + @ids: An array of Desktop IDs (excluding .desktop) connected to geoclue > + --> > + <method name="GetClientList"> This should be a property: Clients
Comment 3 David Hewitt 2017-10-31 23:18:40 UTC
Created attachment 135199 [details] [review] Add clients property to manager Hi Zeeshan, I believe this patch addresses the comments you made on the previous one. The method in which you access the client list is now via a property instead of a method and only agents can access this property. To be able to identify an agent, I had to add a hash set to the manager that keeps track of DBus sender IDs. Agents cannot be XDG applications, so there is no way to use the already built XDG validation as used in gclue_service_client_handle_get_property(). Let me know your thoughts. Thank you, David
Comment 4 GitLab Migration User 2018-05-06 14:37:02 UTC
-- 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/57.