After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 737362 - Privacy panel is missing switch to disable captive portal detection
Privacy panel is missing switch to disable captive portal detection
Status: RESOLVED OBSOLETE
Product: gnome-control-center
Classification: Core
Component: Privacy
unspecified
Other All
: Normal normal
: ---
Assigned To: Rui Matos
Control-Center Maintainers
triaged
Depends on: 785117
Blocks:
 
 
Reported: 2014-09-25 12:50 UTC by Allison Karlitskaya (desrt)
Modified: 2019-01-17 11:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-panels-privacy-add-network-connectivity-checking-tog.patch (10.98 KB, patch)
2017-08-15 10:57 UTC, James Henstridge
none Details | Review
0001-panels-privacy-add-network-connectivity-checking-tog.patch (v2) (11.04 KB, patch)
2017-09-20 08:37 UTC, James Henstridge
none Details | Review
0001-panels-privacy-add-network-connectivity-checking-tog.patch (v3) (10.29 KB, patch)
2017-09-26 20:08 UTC, James Henstridge
none Details | Review
0001-panels-privacy-add-network-connectivity-checking-tog.patch (v4) (9.63 KB, patch)
2018-01-26 05:43 UTC, James Henstridge
reviewed Details | Review
panels/privacy: add network connectivity checking toggle (9.99 KB, patch)
2018-01-26 13:28 UTC, Georges Basile Stavracas Neto
none Details | Review
privacy: Don't define structs defined in unavailable libraries (3.50 KB, patch)
2018-01-26 14:14 UTC, Bastien Nocera
reviewed Details | Review
panels/privacy: add network connectivity checking toggle (10.02 KB, patch)
2018-02-05 13:28 UTC, Jeremy Bicha
none Details | Review
panels/privacy: add network connectivity checking toggle (10.28 KB, patch)
2018-03-09 12:53 UTC, Jeremy Bicha
none Details | Review

Description Allison Karlitskaya (desrt) 2014-09-25 12:50:18 UTC
Since upgrading to Fedora 21 I notice that my computer is attempting to connect to gnome.org whenever I connect to a network.

I would expect to be able to find a way to disable this inside of the Privacy panel.

It should maybe be off by default.

Meanwhile, since it's on with no apparent way to disable it, do we plan to use the data to get an estimate of users?
Comment 1 Bastien Nocera 2014-09-25 12:59:24 UTC
(In reply to comment #0)
> Since upgrading to Fedora 21 I notice that my computer is attempting to connect
> to gnome.org whenever I connect to a network.

You can disable it in /etc/NetworkManager/conf.d/20-connectivity-fedora.conf

> I would expect to be able to find a way to disable this inside of the Privacy
> panel.

GNOME itself doesn't ship any such files.

> It should maybe be off by default.

No, I don't want people to have to unbreak their systems. Even without captive portal support, it's also used to check for connectivity. If you're connecting to a wifi that doesn't route to the Internet, you don't want all your tabs, email, etc. to start trying to fetch data. It's also better in terms of security for users, avoiding incorrect SSL certificate warnings.

> Meanwhile, since it's on with no apparent way to disable it, do we plan to use
> the data to get an estimate of users?

It points to a Fedora server in Fedora. I don't know the privacy policy for that service in Fedora, but it's unlikely that it's one of the allowed uses.
Comment 2 Allison Karlitskaya (desrt) 2014-09-25 13:08:07 UTC
It seems like there are actually two components at play here.

NM itself is configured (at least on my system) to hit:

  http://fedoraproject.org/static/hotspot.txt

but in response to that check failing, gnome-shell will instruct itself via D-Bus (using an apparently non-configurable url) to hit www.gnome.org.

Either way, we should be able to disable both of these via the privacy panel.
Comment 3 Bastien Nocera 2014-09-25 13:15:12 UTC
(In reply to comment #2)
> It seems like there are actually two components at play here.
> 
> NM itself is configured (at least on my system) to hit:
> 
>   http://fedoraproject.org/static/hotspot.txt

Yes, as configured in the file above.

> but in response to that check failing, gnome-shell will instruct itself via
> D-Bus (using an apparently non-configurable url) to hit www.gnome.org.

That's what GNOME does when a captive portal is encountered. You'd get a redirect from www.gnome.org to the captive portal, which will send you back to www.gnome.org once the information is entered (at which point the captive portal helper detects that you're done).

> Either way, we should be able to disable both of these via the privacy panel.

You actually get less security if you don't use it. Disabling NM's connectivity checks will disable both functionality.
Still not interested in handing a way for users to get back to the same broken state that was possible without the connectivity checks.

If restrictions are required for some users (never do any network access without a VPN being on for example), this should be implemented at the NM level, not GNOME, and not in a way that breaks user systems.
Comment 4 Allison Karlitskaya (desrt) 2014-09-25 13:20:24 UTC
> No, I don't want people to have to unbreak their systems. Even without captive
> portal support, it's also used to check for connectivity. If you're connecting
> to a wifi that doesn't route to the Internet, you don't want all your tabs,
> email, etc. to start trying to fetch data. It's also better in terms of
> security for users, avoiding incorrect SSL certificate warnings.

"It's not going to work as nicely without this non-essential feature" is the oldest argument in the book, given by people who are not in a situation where they need to be particularly concerned about their personal privacy.

Some people don't have that luxury, and lacking even an option to disable such things seems exceptionally strange to me from a desktop that is currently trying to position itself as being privacy-friendly.

Also: what happens on the day when Fedora's service provider is having connectivity trouble?  Is there some mechanism that I can use to override the failed result of the check in order to force all of my tabs, email, etc. to load?


Consider how easy it would be to use access (or attempted access) to the above URLs in order to tag certain people as GNOME users.  Would be a really great way to track people who are in a relatively small group.  GNOME is < 1% of the market, right?  If we know that some person we are looking for uses GNOME, then we can instantly reduce our search space by two orders of magnitude.

Also consider how great this information would be in case there is an exploit discovered that impacts GNOME, Fedora, etc.  All I have to do is sit in a busy public place and passively wait until I see requests over open wifi for this URL...

GNOME is depending on NetworkManager, so I don't think we can really just wash our hands of this feature by pushing the blame down the stack.  'Normal users' should not have to know about some file in /etc that they need to edit in order to get their privacy back -- they should be able to find all of this in one place, which is the privacy panel.
Comment 5 Branko Grubic (bitlord) 2014-09-25 13:26:41 UTC
For a fedora (downstream) specific issue about this, I filed a fesco ticket.
I would also like to have this optional, not sure which component is responsible for controlling it (or who decides to implement end-user easily accessible control over this feature). 

# fedora specific
https://fedorahosted.org/fesco/ticket/1337
Comment 6 Bastien Nocera 2014-09-25 13:30:03 UTC
(In reply to comment #4)
> > No, I don't want people to have to unbreak their systems. Even without captive
> > portal support, it's also used to check for connectivity. If you're connecting
> > to a wifi that doesn't route to the Internet, you don't want all your tabs,
> > email, etc. to start trying to fetch data. It's also better in terms of
> > security for users, avoiding incorrect SSL certificate warnings.
> 
> "It's not going to work as nicely without this non-essential feature" is the
> oldest argument in the book, given by people who are not in a situation where
> they need to be particularly concerned about their personal privacy.

That's BS. I already explained to you how it solves a real problem with SSL certificates, bombarding users with "this SSL certificate doesn't match" in Evolution for example.

> Some people don't have that luxury, and lacking even an option to disable such
> things seems exceptionally strange to me from a desktop that is currently
> trying to position itself as being privacy-friendly.

As I already mentioned, it's disablable at the system level.

> Also: what happens on the day when Fedora's service provider is having
> connectivity trouble?  Is there some mechanism that I can use to override the
> failed result of the check in order to force all of my tabs, email, etc. to
> load?

You disable the connectivity check in that file.

> Consider how easy it would be to use access (or attempted access) to the above
> URLs in order to tag certain people as GNOME users.  Would be a really great
> way to track people who are in a relatively small group.  GNOME is < 1% of the
> market, right?  If we know that some person we are looking for uses GNOME, then
> we can instantly reduce our search space by two orders of magnitude.

In which case we should switch Fedora and GNOME's URLs to be part of a bigger group. We could ask Mozilla for their connectivity servers and switch both to that.

> Also consider how great this information would be in case there is an exploit
> discovered that impacts GNOME, Fedora, etc.  All I have to do is sit in a busy
> public place and passively wait until I see requests over open wifi for this
> URL...
> 
> GNOME is depending on NetworkManager, so I don't think we can really just wash
> our hands of this feature by pushing the blame down the stack.  'Normal users'
> should not have to know about some file in /etc that they need to edit in order
> to get their privacy back -- they should be able to find all of this in one
> place, which is the privacy panel.

You say "get privacy back" and I say "give away security". Privacy is nothing without security.
Comment 7 Allison Karlitskaya (desrt) 2014-09-25 13:51:31 UTC
Captive portal checks in no way improve security.  There are two situations that a user could find themselves in:

1) behind a simple innocent captive portal

2) under actual attack

In any case, we should never present the user with the ability to accept a bad SSL certificate.

If we are behind a simple innocent portal then SSL will fail, but that's how it was before now... and there's no real risk here -- even if we present the user with a "bad SSL cert" dialog and they click yes, they're going to try to connect to some server that has no idea what they're talking about.

If we are actually under attack, and the attacker is sophisticated enough to MITM SSL (assuming the user would accept a false cert) then they also _easily_ have the ability to spoof the reply to the captive portal check.  Even if we use https for the check, they could forward the check to Fedora's servers and MITM the other connections.  SSL security only protects a single session.

There is really no "captive portals improve security" argument here.
Comment 8 Matthias Clasen 2014-09-29 12:54:40 UTC
I think the next step here would be for the proponents of this to come up with a patch.
Comment 9 Jeremy Bicha 2017-03-15 23:56:56 UTC
I like Fedora's approach of splitting this config to a separate package so it's relatively easy to disable: just uninstall config-connectivity-fedora.

I've proposed taking the same approach in Ubuntu and Debian (network-manager-config-connectivity is my proposed pkg name).

If most distros do this, couldn't we just use PackageKit to report the status of this feature and turn it on or off?
Comment 10 Laurent Bigonville 2017-07-13 14:42:50 UTC
The problem in debian/ubuntu is that files in /etc are "specials" and the package needs to be purged to have these files removed.
Comment 11 Bastien Nocera 2017-07-13 14:48:17 UTC
(In reply to Laurent Bigonville from comment #10)
> The problem in debian/ubuntu is that files in /etc are "specials" and the
> package needs to be purged to have these files removed.

The file could very well live in /usr/lib/NetworkManager/conf.d
Comment 12 Jeremy Bicha 2017-07-13 15:16:58 UTC
(In reply to Bastien Nocera from comment #11)
> The file could very well live in /usr/lib/NetworkManager/conf.d

In fact, that's what Debian 'testing' does now with their network-manager-config-connectivity-debian package.
Comment 13 James Henstridge 2017-07-19 10:43:51 UTC
I've had a go at adding an API to NetworkManager that could be used to implement this control panel addition as bug 785117.

If that change is accepted, the privacy control panel could (a) detect if a connectivity check service had been configured, and (b) enable or disable it.

With this change, it wouldn't be necessary to ship the connectivity config as a separate package to satisfy the privacy concerns.  It would also remove the need to bake in the name of the package providing the config, as would happen with a PackageKit solution.
Comment 14 James Henstridge 2017-08-15 10:57:22 UTC
Created attachment 357620 [details] [review]
0001-panels-privacy-add-network-connectivity-checking-tog.patch

Attached is a patch that adds support for toggling connectivity checking in the privacy control panel.  It depends on the NetworkManager changes from bug 785117, which haven't landed yet.
Comment 15 Jeremy Bicha 2017-08-17 16:06:24 UTC
Canonical would like this feature to be included in Ubuntu 17.10 but we'd like to get some review first if possible.
Comment 16 Iain Lane 2017-09-08 11:34:49 UTC
Review of attachment 357620 [details] [review]:

'k, I thought I'd review even though I'm not a gnome-control-center maintainer. Most of my comments are nitpicks, just one is an actual bug. To me it's clear that this is a good candidate for a setting in Privacy so I hope it can be accepted.

This requires API in a new version of NetworkManager - please bump the dependency. And also you get deprecation warnings unless you follow the recommendations outlined in https://developer.gnome.org/libnm/stable/usage.html "How to use libnm" - I recommend you do that too.

::: panels/privacy/cc-privacy-panel.c
@@ +1253,3 @@
+static void
+set_connectivity_check_label (NMClient   *client,
+                              GParamSpec *pspec,

G_GNUC_UNUSED

@@ +1276,3 @@
+static void
+set_connectivity_row_visible (NMClient   *client,
+                              GParamSpec *pspec,

G_GNUC_UNUSED

@@ +1286,3 @@
+static void
+set_connectivity_switch (NMClient   *client,
+                         GParamSpec *pspec,

G_GNUC_UNUSED

@@ +1295,3 @@
+
+static gboolean
+connectivity_switch_state_set (GtkSwitch *sw,

This should be called on_connectivity_switch_state_set, for consistency with on_location_app_state_set.

@@ +1300,3 @@
+{
+  nm_client_connectivity_check_set_enabled (client, state);
+  return TRUE;

You should return FALSE here, so the default signal handler runs. This is the cause of https://bugs.launchpad.net/ubuntu/+source/gnome-control-center/+bug/1715662
Comment 17 James Henstridge 2017-09-20 08:37:27 UTC
Created attachment 360104 [details] [review]
0001-panels-privacy-add-network-connectivity-checking-tog.patch (v2)

Sorry for the delay in updating this.  Attached is an updated version of the patch.  The main changes are:

* Use G_GNUC_UNUSED for unused function arguments.
* Hook set_connectivity_switch callback to the correct property.

Returning TRUE from on_connectivity_switch_state_set was correct: the real bug was that the set_connectivity_switch callback was hooked up to the wrong NMClient property, so the delayed gtk_switch_set_state() call wasn't occurring.

The intention was to implement the behaviour described here:

https://developer.gnome.org/gtk3/stable/GtkSwitch.html#GtkSwitch-state-set
Comment 18 Iain Lane 2017-09-20 10:36:13 UTC
(In reply to James Henstridge from comment #17)
> Returning TRUE from on_connectivity_switch_state_set was correct: the real
> bug was that the set_connectivity_switch callback was hooked up to the wrong
> NMClient property, so the delayed gtk_switch_set_state() call wasn't
> occurring.
> 
> The intention was to implement the behaviour described here:
> 
> https://developer.gnome.org/gtk3/stable/GtkSwitch.html#GtkSwitch-state-set

Alright, thanks, that part sounds good to me then.

Hopefully a maintainer can review the patch soon.
Comment 19 Rui Matos 2017-09-20 15:53:00 UTC
Review of attachment 360104 [details] [review]:

Thanks for the patch, it looks generally good but I have some comments

::: panels/privacy/cc-privacy-panel.c
@@ +28,3 @@
 #include <gio/gdesktopappinfo.h>
 #include <glib/gi18n.h>
+#ifdef BUILD_NETWORK

we need to either bump the required NM version to 1.10 in configure.ac or, since this is a neatly contained feature, just check here with the NM_CHECK_VERSION() macro

@@ +1320,3 @@
+                           self->priv->connectivity_check_row, 0);
+  set_connectivity_row_visible (self->priv->nm_client, NULL,
+                                self->priv->connectivity_check_row);

could be simplified by using g_object_bind_property()

@@ +1330,3 @@
+                           "notify::" NM_CLIENT_CONNECTIVITY_CHECK_ENABLED,
+                           G_CALLBACK (set_connectivity_switch), w, 0);
+  set_connectivity_switch (self->priv->nm_client, NULL, GTK_SWITCH (w));

again, I'd prefer g_object_bind_property()

@@ +1331,3 @@
+                           G_CALLBACK (set_connectivity_switch), w, 0);
+  set_connectivity_switch (self->priv->nm_client, NULL, GTK_SWITCH (w));
+  g_signal_connect_object (w, "state-set",

handling GtkSwitch::state-set is only needed if the API we're toggling was async which isn't the case
Comment 20 James Henstridge 2017-09-26 20:08:36 UTC
Created attachment 360490 [details] [review]
0001-panels-privacy-add-network-connectivity-checking-tog.patch (v3)

I've updated the patch to protect the code using NM_CHECK_VERSION(), and make use of g_object_bind_property().

As far as the use of GtkSwitch::state-set, toggling the property invokes a D-Bus method call that will invoke a polkit access check.  That could lead to the user being prompted for auth depending on the configuration, which is why I thought the async update mode seemed appropriate.  Is that not the case?
Comment 21 Rui Matos 2017-11-10 18:03:14 UTC
Review of attachment 360490 [details] [review]:

Sorry for the delay, I still have some questions, see below

::: panels/privacy/cc-privacy-panel.c
@@ +1269,3 @@
+  g_object_bind_property_full (client, NM_CLIENT_CONNECTIVITY_CHECK_ENABLED,
+                               w, "label",
+                               G_BINDING_SYNC_CREATE,

This should be G_BINDING_SYNC_DEFAULT to update the label when the NM prop changes.

@@ +1308,3 @@
+                          w, "state",
+                          G_BINDING_SYNC_CREATE);
+  g_signal_connect_object (w, "state-set",

This doesn't look like a proper usage of the API. We shouldn't need to connect to the signal here, instead, the g_object_bind_property() above should be done on the "active" GtkSwitch property and it should be G_BINDING_BIDIRECTIONAL. Nothing else should be needed.

The state-set signal exists for cases where the GtkSwitch state drives an explicitly async API that we control directly which isn't the case with this NMClient api which is (to us) a simple boolean property.

You raise a good question though, if we write a value to NMClient:connectivity-check-enabled and the NM daemon rejects the change, what happens? Does the NMClient instance automatically get notified and updates its GObject property back to the daemon's value?
Comment 22 Bastien Nocera 2018-01-18 11:56:24 UTC
Review of attachment 360490 [details] [review]:

Note that we currently require NM 1.2, this update would require a much newer version.

I think that we'll probably want to require, at most, the version available in the current stable versions of Ubuntu and Fedora to avoid problems, which means 1.8 for both.

The Makefile.am changes also won't apply to master now, as we've switched to meson.
Comment 23 Jeremy Bicha 2018-01-21 23:30:13 UTC
(In reply to Bastien Nocera from comment #22)
> I think that we'll probably want to require, at most, the version available
> in the current stable versions of Ubuntu and Fedora to avoid problems, which
> means 1.8 for both.

Ubuntu 17.10's NetworkManager already has the D-Bus API backported to it.

It sounds like you're saying you don't want this feature in other distros until GNOME 3.30? I'm not sure that makes much sense here. People shouldn't be trying to use a newer gnome-control-center without being willing to backport other parts of the GNOME stack.

NM 1.10.0 was released in November.
Comment 24 Bastien Nocera 2018-01-21 23:43:32 UTC
(In reply to Jeremy Bicha from comment #23)
> (In reply to Bastien Nocera from comment #22)
> > I think that we'll probably want to require, at most, the version available
> > in the current stable versions of Ubuntu and Fedora to avoid problems, which
> > means 1.8 for both.
> 
> Ubuntu 17.10's NetworkManager already has the D-Bus API backported to it.

Backporting API to older releases? Dangerous game to play.

> It sounds like you're saying you don't want this feature in other distros
> until GNOME 3.30?

No, that you should make its usage a compile-time check, so people can still use the version of NetworkManager just before.

> I'm not sure that makes much sense here. People shouldn't
> be trying to use a newer gnome-control-center without being willing to
> backport other parts of the GNOME stack.

NetworkManager isn't as easy to replace on developers' machines as a library would be.

So it needs to be a compile-time check.
Comment 25 Jeremy Bicha 2018-01-22 00:12:53 UTC
(In reply to Bastien Nocera from comment #24)
> Backporting API to older releases? Dangerous game to play.

I don't think it's necessarily any more dangerous than Fedora backporting new GNOME features to its releases.

> So it needs to be a compile-time check.

Ok, I think that makes sense and is more clear than your original comment. Meanwhile, I opened https://gitlab.gnome.org/GNOME/gnome-build-meta/issues/1 to request that NM 1.10 be allowed for GNOME 3.28.

IIUC, Michael Catanzaro indicated that system dependencies no longer need to be as tied to stable distro releases: 
https://mail.gnome.org/archives/desktop-devel-list/2018-January/msg00030.html
Comment 26 Bastien Nocera 2018-01-22 00:28:42 UTC
(In reply to Jeremy Bicha from comment #25)
> (In reply to Bastien Nocera from comment #24)
> > Backporting API to older releases? Dangerous game to play.
> 
> I don't think it's necessarily any more dangerous than Fedora backporting
> new GNOME features to its releases.

It's dangerous because API users expect a certain API to only be available in a particular version. As you will when you add the compile-time check.

> > So it needs to be a compile-time check.
> 
> Ok, I think that makes sense and is more clear than your original comment.
> Meanwhile, I opened https://gitlab.gnome.org/GNOME/gnome-build-meta/issues/1
> to request that NM 1.10 be allowed for GNOME 3.28.
> 
> IIUC, Michael Catanzaro indicated that system dependencies no longer need to
> be as tied to stable distro releases: 
> https://mail.gnome.org/archives/desktop-devel-list/2018-January/msg00030.html

A library that can be installed in a jhbuild install dir and a daemon that needs root to be installed and working can't be handled in the same way. As a matter of fact, Ubuntu didn't think upgrading NetworkManager was trivial either, which is why the feature was backported instead of the daemon upgraded.
Comment 27 Michael Catanzaro 2018-01-22 00:38:14 UTC
Review of attachment 360490 [details] [review]:

Setting aside the technical concerns of Rui and Bastien, would need to be addressed for this to be committed....

I agree, this setting would be useful. Privacy-conscious users don't want their computers phoning home like this without explicit consent.

::: panels/privacy/cc-privacy-panel.c
@@ +1295,3 @@
+
+  w = get_connectivity_check_label (self->priv->nm_client);
+  self->priv->connectivity_check_row = add_row (self, _("Network Connectivity Checking"), "connectivity_check_dialog", w);

I wonder if we can think of a better name for this label.

Problem is, turning this off is going to seriously break the user experience of accessing networks that use captive portals quite badly. Without the portal helper, you can expect users to get nothing but browser security warnings when trying to load webpages; the only way for a portal to ever display its login page is if the user loads a non-https page, but those are rare enough nowadays that many users never see one, ever. I remember a particular internal bug report from a year or so ago, caused by someone who only tried to use Facebook and Google services, so he could never authenticate to the wireless network, because the portal helper was not enabled. This user thought the problem was that his laptop was broken, and gave up trying to use the internet entirely until we figured it out. So while a technical user like you or me probably immediately knows to load a non-https webpage to access the network, this user never figured that out, and I suspect most users would not be able to either.

Of course, all other apps that use the internet will be broken too. And it can bite technical users too... in a time when the portal helper was less reliable, I wound up debugging a jhbuild crash that turned out to be caused by jhbuild trying to parse a HTML page with a big JPEG of some horses as if it were an XML jhbuild moduleset. Captive portals are wonderful!

So, while the desire to avoid phoning home is understandable, we need to find a way to make it clear to the user why this setting is actually useful. We might call it, for instance "Wi-Fi connection helper" with sublabel "Helps you connect to Wi-Fi networks." Then technical users who understand what this means can disable it, but most users will hopefully be cautious enough to not do so. Problem with this is both my hopes are probably not valid: my proposed label is not clear enough to inform technical users what the functionality really does. So maybe someone can come up with a better proposal...?
Comment 28 James Henstridge 2018-01-26 05:43:25 UTC
Created attachment 367455 [details] [review]
0001-panels-privacy-add-network-connectivity-checking-tog.patch (v4)

Here's an updated version of the patch:

* Ported build changes to Meson
* Don't attempt to do async updates for ConnectivityCheckEnabled

I'm still using G_BINDING_SYNC_CREATE, since I want the UI to match the initial state of NetworkManager.  G_BINDING_DEFAULT is the zero value of the flag set, so it only makes sense to specify it if we don't want any additional behaviours.

The text string changes are the same as before, so that will need changes once there's consensus on what to change to.

The feature is still uses a preprocessor check to make the feature conditional without making NM 1.10 a hard dependency.  It shouldn't be too difficult to make this a hard dep later on.
Comment 29 Georges Basile Stavracas Neto 2018-01-26 13:26:15 UTC
Review of attachment 367455 [details] [review]:

The commit message could have something written, and there are a few improvements to be done in the wording. I'll attach another patch fixing that.
Comment 30 Georges Basile Stavracas Neto 2018-01-26 13:28:56 UTC
Created attachment 367479 [details] [review]
panels/privacy: add network connectivity checking toggle

NetworkManager supports toggling the periodic network check,
a check that by itself can be a security threat since it leaks
information about the host.

This patch adds a periodic network check toggle to the Privacy
panel. This is only enabled when a recent enough NetworkManager
is supported.
Comment 31 Georges Basile Stavracas Neto 2018-01-26 13:29:59 UTC
Attachment 367479 [details] pushed as dbbea7d - panels/privacy: add network connectivity checking toggle
Comment 32 Bastien Nocera 2018-01-26 14:14:51 UTC
Created attachment 367480 [details] [review]
privacy: Don't define structs defined in unavailable libraries

Doing:
typedef struct _NMClient NMClient;
when there's no NMClient available barely makes the code more readable,
and doesn't clear show when it would be used.
Comment 33 Bastien Nocera 2018-01-26 14:15:29 UTC
Comment on attachment 367480 [details] [review]
privacy: Don't define structs defined in unavailable libraries

Attachment 367480 [details] pushed as 484b83f - privacy: Don't define structs defined in unavailable libraries
Comment 34 Bastien Nocera 2018-01-26 14:19:45 UTC
(In reply to Georges Basile Stavracas Neto from comment #30)
> Created attachment 367479 [details] [review] [review]
> panels/privacy: add network connectivity checking toggle
> 
> NetworkManager supports toggling the periodic network check,
> a check that by itself can be a security threat since it leaks
> information about the host.

I've added a note for this one, visible here:
https://git.gnome.org/browse/gnome-control-center/commit/?id=dbbea7ddcb7508d4639b60845e176f396f1029c3
Comment 35 Bastien Nocera 2018-01-26 14:21:21 UTC
I'm going to leave this open because the observations in comment 27 are still valid, which is why I was wary of adding this feature in the first place. It'll likely need some UI changes made.
Comment 36 Sebastien Bacher 2018-01-26 15:12:37 UTC
> Without the portal helper, you can expect users to get nothing but browser
> security warnings when trying to load webpages; the only way for a portal to
> ever display its login page is if the user loads a non-https page, but those
> are rare enough nowadays that many users never see one, ever.

That's less true nowadays, firefox a got captive portal detection feature as well (https://bugzilla.mozilla.org/show_bug.cgi?id=562917) so users get their browser to tell them about the portal now even if they try to visit https websites only
Comment 37 Jeremy Bicha 2018-01-26 15:14:29 UTC
(In reply to Bastien Nocera from comment #35)
> I'm going to leave this open because the observations in comment 27 are
> still valid, which is why I was wary of adding this feature in the first
> place. It'll likely need some UI changes made.

I suggest a new design bug might be better at this point.
Comment 38 Bastien Nocera 2018-01-26 15:35:23 UTC
(In reply to Sebastien Bacher from comment #36)
> > Without the portal helper, you can expect users to get nothing but browser
> > security warnings when trying to load webpages; the only way for a portal to
> > ever display its login page is if the user loads a non-https page, but those
> > are rare enough nowadays that many users never see one, ever.
> 
> That's less true nowadays, firefox a got captive portal detection feature as
> well (https://bugzilla.mozilla.org/show_bug.cgi?id=562917) so users get
> their browser to tell them about the portal now even if they try to visit
> https websites only

Less true, but Firefox is but one browser (at least GNOME's own browser doesn't have that feature, and neither does the popular Chrome), and it wouldn't protect the rest of the system (be it jhbuild or software updates) which would think they're connected.

(In reply to Jeremy Bicha from comment #37)
> (In reply to Bastien Nocera from comment #35)
> > I'm going to leave this open because the observations in comment 27 are
> > still valid, which is why I was wary of adding this feature in the first
> > place. It'll likely need some UI changes made.
> 
> I suggest a new design bug might be better at this point.

I'm afraid not, because this is a blocking problem, and we'd likely revert the patches (or simply toggle the #define in my latest patch) if we can't resolve this.

If you wanted to ship this functionality despite the design problems, then at least it's just a one line patch to enable.
Comment 39 Georges Basile Stavracas Neto 2018-01-26 18:29:50 UTC
I reverted these changes after realising this was a mistake. Let's at the very least agree on the wording before landing anything.
Comment 40 Iain Lane 2018-01-30 17:10:51 UTC
From chatting on IRC (#control-center) and some comments on this bug, we need to come up with some text which indicates what is potentially 'bad' about the traffic and what might break. That's a tall order but I hope it's not insurmountable...

Can we chat about some specific text maybe? Here's something potentially slightly clunky to get us started:

Checking for connectivity allows networks which require authentication to be detected, but it generates occasional traffic that may be monitored to reveal your operating system and when you are online. If disabled, some applications may fail in unexpected ways when the network is not authenticated to.
Comment 41 Allan Day 2018-02-01 11:56:25 UTC
(In reply to Georges Basile Stavracas Neto from comment #39)
> I reverted these changes after realising this was a mistake. Let's at the
> very least agree on the wording before landing anything.

I've added the connectivity checking dialog to the privacy panel mockups. They include a suggested wording.

https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/privacy/privacy.png
Comment 42 Bastien Nocera 2018-02-01 13:04:57 UTC
As mentioned on IRC, I would mention that you shouldn't disable it unless you know what you're doing, and I wouldn't put it up top in that section either, as it is the least important of the settings.
Comment 43 Jeremy Bicha 2018-02-05 13:28:27 UTC
Created attachment 367910 [details] [review]
panels/privacy: add network connectivity checking toggle

NetworkManager supports toggling the periodic network check,
a check that by itself can be a security threat since it leaks
information about the host.

This patch adds a periodic network check toggle to the Privacy
panel. This is only enabled when a recent enough NetworkManager
is supported.

I have updated the patch to use the wording suggested in Allan's mockup from comment #41. I have left the Connectivity switch at the bottom of the panel as originally implemented in Ubuntu instead of at the top of the list.
Comment 44 Allan Day 2018-02-06 17:51:37 UTC
(In reply to Jeremy Bicha from comment #43)
...
> I have updated the patch to use the wording suggested in Allan's mockup from
> comment #41. I have left the Connectivity switch at the bottom of the panel
> as originally implemented in Ubuntu instead of at the top of the list.

I think that I need to clarify my position on this issue.

I was only asked to advise on the issue very recently. At that point, code had been written and it was presented to me as something that was about to land. When I added the connectivity check setting to the mockups, it was largely on the basis of trying to polish something that was happening one way or another.

However, if I'm honest, I'm not entirely convinced whether this setting should be part of the control center, and I'm yet to see a clearly articulated rationale for including it, in terms of:

 1. common use cases for when it would be used
 2. how it can be effectively communicated to users
 3. what the side-effects on user experience might be, and how they might be mitigated

I'm honestly not sure what the status of this bug is, but from my perspective it might be best if we could address the three points above, before spending time on the wording of the dialog.
Comment 45 Jeremy Bicha 2018-02-06 18:01:56 UTC
(In reply to Allan Day from comment #44)
> I was only asked to advise on the issue very recently.

Probably irrelevant now, but we first pinged you about this on August 15. It's frustrating that these concerns were presented so late (January 21 from what I can see).
Comment 46 Bastien Nocera 2018-02-06 18:11:27 UTC
(In reply to Jeremy Bicha from comment #45)
> (In reply to Allan Day from comment #44)
> > I was only asked to advise on the issue very recently.
> 
> Probably irrelevant now, but we first pinged you about this on August 15.
> It's frustrating that these concerns were presented so late (January 21 from
> what I can see).

I said in 2014 that I didn't like the feature, if we're going for historical records. The concerns of the time were never alleviated. At least now you have a patch ready to include in your distribution if you so choose.
Comment 47 Allan Day 2018-02-06 19:28:29 UTC
(In reply to Jeremy Bicha from comment #45)
> (In reply to Allan Day from comment #44)
> > I was only asked to advise on the issue very recently.
> 
> Probably irrelevant now, but we first pinged you about this on August 15.
> It's frustrating that these concerns were presented so late (January 21 from
> what I can see).

I'm genuinely sorry if I missed something. I try to make sure that I don't drop issues that are brought to me, but I'm just one person and things do sometimes slip through the cracks. Last summer was particularly busy for me.

All I want to do is ensure that a UX-driven process has been adhered to - for a new feature, I would expect there to be a clear rationale somewhere. I'm happy to help in examining that, and in any further design work that might be required.
Comment 48 Jeremy Bicha 2018-03-09 12:53:57 UTC
Created attachment 369505 [details] [review]
panels/privacy: add network connectivity checking toggle

(fix typo)
Comment 49 Jeremy Bicha 2019-01-16 23:36:03 UTC
I updated the patch for current git master and proposed it at https://gitlab.gnome.org/GNOME/gnome-control-center/merge_requests/348
Comment 50 Bastien Nocera 2019-01-17 11:33:06 UTC
Closing as moved then.