Prevent networkingPrivate.forgetNetwork from removing shared configs

Prevent networkingPrivate.forgetNetwork from removing shared configs

Changes networkingPrivate.forgetNetwork so it only removes network
configs from user network profile, and keeps shared profile intact.
This is part of the effort to restrict networkingPrivate API network
config changes from affecting non-kiosk users sessions once the API
is opened up to kiosk sessions.

Note that networking Web UI behavior should stay the same - when the
API function is called from the Web UI context, the network's configs
should be removed from all profiles that contain the network service.

While here, prevent the API from removing policy controlled configs
(to be consistent with the UX defined in network settings, where policy
controlled configs are not allowed to be forgotten).

In order to properly test the behavior, the patch updates fake shill
service and profile clients to better handle network services defined in
multiple profiles:
* FakeShilProfileClient::GetService now applies both shared and user
profile properties, setting profile path to user profile if the user
profile contains the service
* FakeShillServiceClient::GetLoadableProfileEntries now reports all
profiles that contain an entry for the service, rather than just the
profile to which service's profile_path is set.

BUG=700131, 705024
TBR=blundell@chromium.org
(for components/sync_wifi)

Review-Url: https://codereview.chromium.org/2754903002
Cr-Commit-Position: refs/heads/master@{#460936}
Committed: https://chromium.googlesource.com/chromium/src/+/d0203954892c94a15b4f5843bdc519b2be81f0bc

[tbarzic, 2017-03-28 06:01:07]

Description was changed from

==========
Prevent networkingPrivate.forgetNetwork from removing shared configs

Changes networkingPrivate.forgetNetwork so it only removes network
configs from user network profile, and keeps shared profile intact.
This is part of the effort to restrict networkingPrivate API network
config changes from affecting non-kiosk users sessions once the API
is opened up to kiosk sessions.

Note that networking Web UI behavior should stay the same - when the
API function is called from the Web UI context, the network's configs
should be removed from all profiles that contain the network service.

While here, prevent the API from removing policy controlled configs
(to be consistent with the UX defined in network settings, where policy
controlled configs are not allowed to be forgotten).

In order to properly test the behavior, the patch updates fake shill
service and profile clients to better handle network services defined in
multiple profiles:
* FakeShilProfileClient::GetService now applies both shared and user
profile properties, setting profile path to user profile if the user
profile contains the service
* FakeShillServiceClient::GetLoadableProfileEntries now reports all
profiles that contain an entry for the service, rather than just the
profile to which service's profile_path is set.

BUG=700131
==========

to

==========
Prevent networkingPrivate.forgetNetwork from removing shared configs

Changes networkingPrivate.forgetNetwork so it only removes network
configs from user network profile, and keeps shared profile intact.
This is part of the effort to restrict networkingPrivate API network
config changes from affecting non-kiosk users sessions once the API
is opened up to kiosk sessions.

Note that networking Web UI behavior should stay the same - when the
API function is called from the Web UI context, the network's configs
should be removed from all profiles that contain the network service.

While here, prevent the API from removing policy controlled configs
(to be consistent with the UX defined in network settings, where policy
controlled configs are not allowed to be forgotten).

In order to properly test the behavior, the patch updates fake shill
service and profile clients to better handle network services defined in
multiple profiles:
* FakeShilProfileClient::GetService now applies both shared and user
profile properties, setting profile path to user profile if the user
profile contains the service
* FakeShillServiceClient::GetLoadableProfileEntries now reports all
profiles that contain an entry for the service, rather than just the
profile to which service's profile_path is set.

BUG=700131, 705024
==========

[tbarzic, 2017-03-28 06:20:50]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 06:21:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 07:45:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 07:45:48]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2017-03-28 23:40:22]

tbarzic@chromium.org changed reviewers:
+ stevenjb@chromium.org

[tbarzic, 2017-03-28 23:40:22]

Can you please take a look.

I checked shill code, and it looks like it should handle the situation where
network is removed from it's current profile (i.e. network_state->profile_path()
one), so I changed the new network removal method's semantics a bit (to remove
the configuration from the current profile, rather than allowing removal from
any profile).

Btw. profile entry removal should be tested by shill unittests:

*
https://cs.corp.google.com/chromeos_public/src/aosp/system/connectivity/shill...

*
https://cs.corp.google.com/chromeos_public/src/aosp/system/connectivity/shill...

These (in particular the manager_unittest.cc one) seem to verify that network is
configured in another profile when the associated entry is removed a profile
referenced by .

[stevenjb, 2017-03-29 00:06:46]

Just went through the fake shill changes so far.

https://codereview.chromium.org/2754903002/diff/60001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_profile_client.cc (right):

https://codereview.chromium.org/2754903002/diff/60001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_profile_client.cc:226: }
blank line

https://codereview.chromium.org/2754903002/diff/60001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_profile_client.cc:237: }
So, I kind of hate to ask this, but this is ugly and confusing and I worry about
the next person who comes along and looks at this code.

We should make profiles_ a list instead of a map, and make the path part of
ProfileProperties. It will always be a short list, so iterating or using
std::find() will be fast, and it will simplify this code which should just
iterate through the list and apply the profiles in order (which is what Shill
does IIRC). Then we just need to make sure the shared profile gets added first.

https://codereview.chromium.org/2754903002/diff/60001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_profile_client.cc:269: const ProfileMap::iterator&
iter,
I would rather pass the profile path and const ProfileProperties* separately
than pass an iterator.

[tbarzic, 2017-03-29 01:48:28]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[tbarzic, 2017-03-29 01:48:50]

https://codereview.chromium.org/2754903002/diff/60001/chromeos/dbus/fake_shil...
File chromeos/dbus/fake_shill_profile_client.cc (right):

https://codereview.chromium.org/2754903002/diff/60001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_profile_client.cc:226: }
On 2017/03/29 00:06:45, stevenjb wrote:
> blank line

Done.

https://codereview.chromium.org/2754903002/diff/60001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_profile_client.cc:237: }
On 2017/03/29 00:06:45, stevenjb wrote:
> So, I kind of hate to ask this, but this is ugly and confusing and I worry
about
> the next person who comes along and looks at this code.
> 
> We should make profiles_ a list instead of a map, and make the path part of
> ProfileProperties. It will always be a short list, so iterating or using
> std::find() will be fast, and it will simplify this code which should just
> iterate through the list and apply the profiles in order (which is what Shill
> does IIRC). Then we just need to make sure the shared profile gets added
first.

Sounds reasonable. Done.

https://codereview.chromium.org/2754903002/diff/60001/chromeos/dbus/fake_shil...
chromeos/dbus/fake_shill_profile_client.cc:269: const ProfileMap::iterator&
iter,
On 2017/03/29 00:06:45, stevenjb wrote:
> I would rather pass the profile path and const ProfileProperties* separately
> than pass an iterator.

Done in sense that now only ProfileProperties are needed.

[commit-bot: I haz the power, 2017-03-29 01:49:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 03:51:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 03:51:05]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[stevenjb, 2017-03-30 17:33:14]

https://codereview.chromium.org/2754903002/diff/100001/chromeos/dbus/fake_shi...
File chromeos/dbus/fake_shill_profile_client.cc (right):

https://codereview.chromium.org/2754903002/diff/100001/chromeos/dbus/fake_shi...
chromeos/dbus/fake_shill_profile_client.cc:139: }
nit: We shouldn't need to support multiple shared profiles either, so this could
just be:
CHECK(profile_path != GetSharedProfilePath() || profiles_.empty()) <<
  "The shared profile must be added first.";

https://codereview.chromium.org/2754903002/diff/100001/extensions/browser/api...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2754903002/diff/100001/extensions/browser/api...
extensions/browser/api/networking_private/networking_private_chromeos.cc:485:
onc_source == onc::ONC_SOURCE_USER_POLICY) {
This logic is confusing to follow. I guess it is saying that if we are
restricting removal to the current profile and it is a device policy then the
removal is OK? In that case, shouldn't we set |allow_forget_shared_config| =
false if onc_source == onc::ONC_SOURCE_DEVICE_POLICY instead? i.e.

if (onc_source == onc::ONC_SOURCE_DEVICE_POLICY)
  allow_forget_shared_config = false;
else
  fail and return

[tbarzic, 2017-03-30 18:00:05]

https://codereview.chromium.org/2754903002/diff/100001/chromeos/dbus/fake_shi...
File chromeos/dbus/fake_shill_profile_client.cc (right):

https://codereview.chromium.org/2754903002/diff/100001/chromeos/dbus/fake_shi...
chromeos/dbus/fake_shill_profile_client.cc:139: }
On 2017/03/30 17:33:13, stevenjb wrote:
> nit: We shouldn't need to support multiple shared profiles either, so this
could
> just be:
> CHECK(profile_path != GetSharedProfilePath() || profiles_.empty()) <<
>   "The shared profile must be added first.";
> 
> 

Done.

https://codereview.chromium.org/2754903002/diff/100001/extensions/browser/api...
File extensions/browser/api/networking_private/networking_private_chromeos.cc
(right):

https://codereview.chromium.org/2754903002/diff/100001/extensions/browser/api...
extensions/browser/api/networking_private/networking_private_chromeos.cc:485:
onc_source == onc::ONC_SOURCE_USER_POLICY) {
On 2017/03/30 17:33:13, stevenjb wrote:
> This logic is confusing to follow. I guess it is saying that if we are
> restricting removal to the current profile and it is a device policy then the
> removal is OK? In that case, shouldn't we set |allow_forget_shared_config| =
> false if onc_source == onc::ONC_SOURCE_DEVICE_POLICY instead? i.e.
> 
> if (onc_source == onc::ONC_SOURCE_DEVICE_POLICY)
>   allow_forget_shared_config = false;
> else
>   fail and return

yeah, good point. Done.

[stevenjb, 2017-03-30 18:02:15]

lgtm

[tbarzic, 2017-03-30 18:14:29]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2017-03-30 18:15:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 18:30:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 18:30:34]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tbarzic, 2017-03-30 20:32:34]

Description was changed from

==========
Prevent networkingPrivate.forgetNetwork from removing shared configs

Changes networkingPrivate.forgetNetwork so it only removes network
configs from user network profile, and keeps shared profile intact.
This is part of the effort to restrict networkingPrivate API network
config changes from affecting non-kiosk users sessions once the API
is opened up to kiosk sessions.

Note that networking Web UI behavior should stay the same - when the
API function is called from the Web UI context, the network's configs
should be removed from all profiles that contain the network service.

While here, prevent the API from removing policy controlled configs
(to be consistent with the UX defined in network settings, where policy
controlled configs are not allowed to be forgotten).

In order to properly test the behavior, the patch updates fake shill
service and profile clients to better handle network services defined in
multiple profiles:
* FakeShilProfileClient::GetService now applies both shared and user
profile properties, setting profile path to user profile if the user
profile contains the service
* FakeShillServiceClient::GetLoadableProfileEntries now reports all
profiles that contain an entry for the service, rather than just the
profile to which service's profile_path is set.

BUG=700131, 705024
==========

to

==========
Prevent networkingPrivate.forgetNetwork from removing shared configs

Changes networkingPrivate.forgetNetwork so it only removes network
configs from user network profile, and keeps shared profile intact.
This is part of the effort to restrict networkingPrivate API network
config changes from affecting non-kiosk users sessions once the API
is opened up to kiosk sessions.

Note that networking Web UI behavior should stay the same - when the
API function is called from the Web UI context, the network's configs
should be removed from all profiles that contain the network service.

While here, prevent the API from removing policy controlled configs
(to be consistent with the UX defined in network settings, where policy
controlled configs are not allowed to be forgotten).

In order to properly test the behavior, the patch updates fake shill
service and profile clients to better handle network services defined in
multiple profiles:
* FakeShilProfileClient::GetService now applies both shared and user
profile properties, setting profile path to user profile if the user
profile contains the service
* FakeShillServiceClient::GetLoadableProfileEntries now reports all
profiles that contain an entry for the service, rather than just the
profile to which service's profile_path is set.

BUG=700131, 705024

TBR=blundell@chromium.org
(for components/sync_wifi)
==========

[tbarzic, 2017-03-30 20:32:35]

tbarzic@chromium.org changed reviewers:
+ blundell@chromium.org

[tbarzic, 2017-03-30 20:32:41]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2017-03-30 20:34:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 23:51:38]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490905961832270,
"parent_rev": "de7a2b290555d5db85b3eb4103df2a71a6b06356", "commit_rev":
"d0203954892c94a15b4f5843bdc519b2be81f0bc"}

[commit-bot: I haz the power, 2017-03-30 23:53:08]

Description was changed from

==========
Prevent networkingPrivate.forgetNetwork from removing shared configs

Changes networkingPrivate.forgetNetwork so it only removes network
configs from user network profile, and keeps shared profile intact.
This is part of the effort to restrict networkingPrivate API network
config changes from affecting non-kiosk users sessions once the API
is opened up to kiosk sessions.

Note that networking Web UI behavior should stay the same - when the
API function is called from the Web UI context, the network's configs
should be removed from all profiles that contain the network service.

While here, prevent the API from removing policy controlled configs
(to be consistent with the UX defined in network settings, where policy
controlled configs are not allowed to be forgotten).

In order to properly test the behavior, the patch updates fake shill
service and profile clients to better handle network services defined in
multiple profiles:
* FakeShilProfileClient::GetService now applies both shared and user
profile properties, setting profile path to user profile if the user
profile contains the service
* FakeShillServiceClient::GetLoadableProfileEntries now reports all
profiles that contain an entry for the service, rather than just the
profile to which service's profile_path is set.

BUG=700131, 705024

TBR=blundell@chromium.org
(for components/sync_wifi)
==========

to

==========
Prevent networkingPrivate.forgetNetwork from removing shared configs

Changes networkingPrivate.forgetNetwork so it only removes network
configs from user network profile, and keeps shared profile intact.
This is part of the effort to restrict networkingPrivate API network
config changes from affecting non-kiosk users sessions once the API
is opened up to kiosk sessions.

Note that networking Web UI behavior should stay the same - when the
API function is called from the Web UI context, the network's configs
should be removed from all profiles that contain the network service.

While here, prevent the API from removing policy controlled configs
(to be consistent with the UX defined in network settings, where policy
controlled configs are not allowed to be forgotten).

In order to properly test the behavior, the patch updates fake shill
service and profile clients to better handle network services defined in
multiple profiles:
* FakeShilProfileClient::GetService now applies both shared and user
profile properties, setting profile path to user profile if the user
profile contains the service
* FakeShillServiceClient::GetLoadableProfileEntries now reports all
profiles that contain an entry for the service, rather than just the
profile to which service's profile_path is set.

BUG=700131, 705024

TBR=blundell@chromium.org
(for components/sync_wifi)

Review-Url: https://codereview.chromium.org/2754903002
Cr-Commit-Position: refs/heads/master@{#460936}
Committed:
https://chromium.googlesource.com/chromium/src/+/d0203954892c94a15b4f5843bdc5...
==========

[commit-bot: I haz the power, 2017-03-30 23:53:09]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d0203954892c94a15b4f5843bdc5...

[blundell, 2017-03-31 08:52:49]

lgtm