GNOME Bugzilla – Bug 785117
Add support for disabling connectivity checking via the D-Bus interface
Last modified: 2017-12-07 07:42:39 UTC
This is an attempt at adding an API to allow a privacy control panel to disable connectivity checking. I've uploaded my branch to github, and will attach the patches shortly: https://github.com/NetworkManager/NetworkManager/compare/master...jhenstridge:connectivity-disable The basic design is to add two new boolean D-Bus properties: ConnectivityCheckAvailable and ConnectivityCheckEnabled. The first is read-only, and will be true if a connectivity check URI has been configured. The second is read/write, and says whether connectivity checking is enabled. Writing to ConnectivityCheckEnabled is handled by setting the connectivity check interval to 0 in NetworkManager-intern.conf, or clearing the change otherwise. The intent is that a privacy control panel would use ConnectivityCheckAvailable to decide whether to display relevant UI, and ConnectivityCheckEnabled would be tied to the toggle.
Created attachment 355928 [details] [review] 0001-config-add-an-API-to-disable-connectivity-check-via-.patch
Created attachment 355929 [details] [review] 0002-manager-add-connectivity-check-available-enabled-pro.patch
Created attachment 355930 [details] [review] 0003-client-expose-connectivity-check-available-enabled-p.patch
would it be possible to adjust indentation to follow our style? See CONTRIBUTING file: »···c) spaces used to align continuation lines past the indent point of the »··· first statement line, like so: +void nm_config_set_connectivity_check_enabled (NMConfig *self, ^^^^^^ new line + gboolean enabled) nm_config_set_connectivity_check_enabled() sets the interval property. That means, 1) distribution installs configuration snippet wit interval=60 2) user disables check via D-Bus 3) distribution wants to update the interval to 45 seconds this causes a package update to overwrite the user configuration. Basically, it means the distro cannot update te original interval without re-enabling connectivity check for all users. That seems bad. Maybe this should be avoided by adding a new configuration option "connectivity.enabled", and let nm_config_set_connectivity_check_enabled() set that. + g_value_set_boolean (value, nm_config_data_get_connectivity_uri (config_data) != NULL); + break; + case PROP_CONNECTIVITY_CHECK_ENABLED: + config_data = nm_config_get_data (priv->config); + g_value_set_boolean (value, + nm_config_data_get_connectivity_uri (config_data) != NULL && + nm_config_data_get_connectivity_interval (config_data) != 0); NMConnectivity already has a complicated notion of what it means when connectivity check is enabled. See nm_connectivity_check_enabled(). E.g. it would reject an empty URI "" or an URI that doesn't have scheme HTTP/HTTPS. Here the logic is duplicated (differently). Probably, it should strictly refer to NMConnectivity. Does "manager: add connectivity-check-{available,enabled} properties." even work? It would seem you also need to whitelist it in nm-manager.c's prop_filter() and prop_set_auth_done_cb(). Possibly also add a new policiy-kit permission, analog to NM_AUTH_PERMISSION_SETTINGS_MODIFY_GLOBAL_DNS.
Created attachment 357233 [details] [review] 0001-config-add-an-API-to-disable-connectivity-check-via-.patch (v2)
Created attachment 357234 [details] [review] 0002-manager-add-connectivity-check-available-enabled-pro.patch (v2)
Created attachment 357235 [details] [review] 0003-client-expose-connectivity-check-available-enabled-p.patch (v2)
Sorry for the delay in getting back to you. I've updated the patches based on your feedback. I think I've got the white space issues solved. I noticed the files had emacs formatting settings at the top, so wrongly assumed it would do the right thing. I've updated the config logic to use a separate "enabled" key, so as not to interfere with OS config updates. The NMConnectivity class now also takes this into account. The NMManager class also uses NMConnectivity when returning the ConnectivityCheckEnabled property rather than repeating the logic. ConnectivityCheckAvailable is still simply checking connectivity_uri. I suppose we could move some of that logic NMConnectivity though. What do you think? I've added in the polkit check for the new property using a new enable-disable-connectivity-check action. I hadn't noticed any problems when testing the first version of my patches, where I hadn't updated the property filter. So I guess that property filter code fails open for new properties.
Created attachment 357616 [details] [review] 0003-client-expose-connectivity-check-available-enabled-p.patch (v3) Fixing a minor bug in the last patch from the previous set: I had forgotten to expose the new methods in the symbol version file, so it was only possible to access them as GObject properties.
merged to master as https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2eb02543c2cee77619788dc162e11b97fe7e4ea8 Nice work! Merged including two minor follow-up patches. If there are issues, please reopen. Thanks.
(In reply to James Henstridge from comment #9) > Created attachment 357616 [details] [review] [review] > 0003-client-expose-connectivity-check-available-enabled-p.patch (v3) > > Fixing a minor bug in the last patch from the previous set: I had forgotten > to expose the new methods in the symbol version file, so it was only > possible to access them as GObject properties. hm, v3 somehow still misses the changes to the symbol version file. (which I didn't notice before merging). Fixed now: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=75aa3ea19451ba88942dd12d5ca0019518d6e747
Sorry about that. It looks like I forgot to rerun "git format-patch" after fixing up the symbol version file. Looking at the change you made, I don't think it is necessary to include the nm_manager_* symbols: none of the others from that class appear to be exported, and I didn't need them when writing the gnome-control-center patch. Thanks for the help in getting this merged.
(In reply to James Henstridge from comment #12) > Sorry about that. It looks like I forgot to rerun "git format-patch" after > fixing up the symbol version file. > > Looking at the change you made, I don't think it is necessary to include the > nm_manager_* symbols: none of the others from that class appear to be > exported, and I didn't need them when writing the gnome-control-center patch. > > Thanks for the help in getting this merged. ah, you are right. libnm/nm-manager.h is a private header file, hence the symbols should not be exported. Fixed: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=a3e84daf44be39d640ef030efaf846b7094f1f72 also: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=804b2c23650a9dc25d7d8030f7cb6962afdbd43c https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=13f955db58bd60a7f5a8e5600c7de62ee9a4c876
Could someone give an status update on this bug? Currently, the status is listed as "resolved fixed," but I not sure if the fix has been approved and merged.
(In reply to Ian S. McBride from comment #14) > Could someone give an status update on this bug? Currently, the status is > listed as "resolved fixed," but I not sure if the fix has been approved and > merged. as said in previous comments, all patches are merged upstream and part of 1.10 or newer.