Handle prohibited technologies in device policy ONC

Handle prohibited technologies in device policy ONC

This commit changes prohibited technologies from user policy ONC to device
policy ONC, and then handles it.

The device policy ONC will only take effect after user session starts.
After use has logged in, the prohibited technologies in ONC will take effect
and disable and prohibit specific technologies.

BUG=426390
Committed: https://crrev.com/fee5067aaad72d7b0525d23d5f6eb2500831feab
Cr-Commit-Position: refs/heads/master@{#359602}

[fqj, 2015-11-03 15:14:04]

Description was changed from

==========
Handle prohibited technologies in device policy ONC

This commit changes prohibited technologies from user policy ONC to device
policy ONC, and then handles it.

The device policy ONC will only take effect after user session starts.
After use has logged in, the prohibited technologies in ONC will take effect
and disable and prohibit specific technologies.

BUG=426390
==========

to

==========
Handle prohibited technologies in device policy ONC

This commit changes prohibited technologies from user policy ONC to device
policy ONC, and then handles it.

The device policy ONC will only take effect after user session starts.
After use has logged in, the prohibited technologies in ONC will take effect
and disable and prohibit specific technologies.

BUG=426390
==========

[fqj, 2015-11-10 14:43:46]

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

[fqj, 2015-11-10 14:43:46]

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

[fqj, 2015-11-10 14:43:46]

PTAL.

[fqj, 2015-11-10 14:43:47]

PTAL.

[stevenjb, 2015-11-10 21:40:21]

I would really prefer to avoid adding this much complexity to
ShillPropertyHandler. SPH should not be keeping track of Chrome state
(in_user_session_ and user_policy_applied_).

Instead, we should rely on policy code to set or unset prohibited technologies
and keep the NSH/SPH code as simple as possible. If we need to introduce another
class for this, that's fine.

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:435:
DCHECK_EQ(::onc::ONC_SOURCE_DEVICE_POLICY, onc_source);
Shouldn't this be checking for USER_POLICY? Also, is this enforced in the
validator? If not we shouldn't DCHECK here, just log an error.

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:443:
network_util::TranslateONCTypeToShill(prohibited_technology));
We should validate the result of TranslateONCTypeToShill to avoid passing empty
values to Shill.

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:449:
network_state_handler_->UserPolicyApplied();
I don't think we should need this extra call, but also see my meta comment.

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:745:
std::vector<std::string>(), chromeos::network_handler::ErrorCallback());
This is not the right place to do this. We should handle this in NSH::Init (if
at all).

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.h (right):

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:46: public LoginState::Observer {
I really don't like adding this dependency here, or the added complication to
this class. I think we should put these changes elsewhere.

[fqj, 2015-11-11 16:53:55]

PTAL

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:435:
DCHECK_EQ(::onc::ONC_SOURCE_DEVICE_POLICY, onc_source);
On 2015/11/10 21:40:21, stevenjb wrote:
> Shouldn't this be checking for USER_POLICY? Also, is this enforced in the
> validator? If not we shouldn't DCHECK here, just log an error.

I sent you a new PRD serveral days ago. It describes now it should be a device
policy only taking effect on user session.
ONC change to make it a device policy is also in this CL.
It's enforced in the validator.

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:443:
network_util::TranslateONCTypeToShill(prohibited_technology));
On 2015/11/10 21:40:21, stevenjb wrote:
> We should validate the result of TranslateONCTypeToShill to avoid passing
empty
> values to Shill.

Done.

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:449:
network_state_handler_->UserPolicyApplied();
On 2015/11/10 21:40:21, stevenjb wrote:
> I don't think we should need this extra call, but also see my meta comment.

Done.

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:745:
std::vector<std::string>(), chromeos::network_handler::ErrorCallback());
On 2015/11/10 21:40:21, stevenjb wrote:
> This is not the right place to do this. We should handle this in NSH::Init (if
> at all).

Moved to the new ProhibitedTechnologiesHandler::Init

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/shill_...
File chromeos/network/shill_property_handler.h (right):

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/shill_...
chromeos/network/shill_property_handler.h:46: public LoginState::Observer {
On 2015/11/10 21:40:21, stevenjb wrote:
> I really don't like adding this dependency here, or the added complication to
> this class. I think we should put these changes elsewhere.

Done.

[stevenjb, 2015-11-11 18:07:31]

This is much more clear, thanks.

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/1431563005/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:435:
DCHECK_EQ(::onc::ONC_SOURCE_DEVICE_POLICY, onc_source);
On 2015/11/11 16:53:54, fqj wrote:
> On 2015/11/10 21:40:21, stevenjb wrote:
> > Shouldn't this be checking for USER_POLICY? Also, is this enforced in the
> > validator? If not we shouldn't DCHECK here, just log an error.
> 
> I sent you a new PRD serveral days ago. It describes now it should be a device
> policy only taking effect on user session.
> ONC change to make it a device policy is also in this CL.
> It's enforced in the validator.

I saw that, I just wanted to make sure that this isn't something that could
happen with a misconfigured ONC file.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/manag...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/manag...
chromeos/network/managed_network_configuration_handler_impl.cc:439: // Build up
prohibited network type list and update NetworkStateHandler.
Comment out of date.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/manag...
chromeos/network/managed_network_configuration_handler_impl.cc:446: if
(translated_tech.length())
!translated_tech.empty()

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/manag...
chromeos/network/managed_network_configuration_handler_impl.cc:450:
prohibited_technologies);
Since we have a separate handler now we should move as much work as possible
there, i.e. pass a ListValue* to SetProhibitedTechnologies.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:152:
technology_list.push_back(prohibited_technology);
Why are we copying a vector<string> to a vector<string>?

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:13: :
managed_network_configuration_handler_(nullptr){};
Need to intialize other members. Better yet, use Non-Static Class Member
Initializers in the header.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:27: }
nit: no {}

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:58:
EnforceProhibitedTechnologies(user_logged_in_ && user_policy_applied_);
&& user_policy_applied_ is unnecessary since it is set above.
Also, since these are members, maybe just check them in
EnforceProhibitedTechnologies() reather than pass this every time?

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:83: else
no else after return

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler.h (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.h:51: bool
user_policy_applied_;
Empty line

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler_unittest.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler_unittest.cc:29: 
no blank line

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:12: #include
"base/strings/string_util.h"
not needed?

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:22: #include
"chromeos/network/network_handler_callbacks.h"
Not needed

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:164: technology);
Align

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:186: // Remove technologies from the
other lists.
Update / move comment

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:197: iter_prohibited_technology++;
{}

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:198: }
Why not just clear prohibited_technologies_ and replace it with
prohibited_technologies?

[fqj, 2015-11-12 10:23:02]

PTAL

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/manag...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/manag...
chromeos/network/managed_network_configuration_handler_impl.cc:439: // Build up
prohibited network type list and update NetworkStateHandler.
On 2015/11/11 18:07:31, stevenjb wrote:
> Comment out of date.

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/manag...
chromeos/network/managed_network_configuration_handler_impl.cc:446: if
(translated_tech.length())
On 2015/11/11 18:07:31, stevenjb wrote:
> !translated_tech.empty()

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/manag...
chromeos/network/managed_network_configuration_handler_impl.cc:450:
prohibited_technologies);
On 2015/11/11 18:07:31, stevenjb wrote:
> Since we have a separate handler now we should move as much work as possible
> there, i.e. pass a ListValue* to SetProhibitedTechnologies.

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:152:
technology_list.push_back(prohibited_technology);
On 2015/11/11 18:07:31, stevenjb wrote:
> Why are we copying a vector<string> to a vector<string>?

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:13: :
managed_network_configuration_handler_(nullptr){};
On 2015/11/11 18:07:31, stevenjb wrote:
> Need to intialize other members. Better yet, use Non-Static Class Member
> Initializers in the header.

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:27: }
On 2015/11/11 18:07:31, stevenjb wrote:
> nit: no {}

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:58:
EnforceProhibitedTechnologies(user_logged_in_ && user_policy_applied_);
On 2015/11/11 18:07:31, stevenjb wrote:
> && user_policy_applied_ is unnecessary since it is set above.
> Also, since these are members, maybe just check them in
> EnforceProhibitedTechnologies() reather than pass this every time?

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:83: else
On 2015/11/11 18:07:31, stevenjb wrote:
> no else after return

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler_unittest.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler_unittest.cc:29: 
On 2015/11/11 18:07:31, stevenjb wrote:
> no blank line

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:12: #include
"base/strings/string_util.h"
On 2015/11/11 18:07:31, stevenjb wrote:
> not needed?

Needed.

  // Send updated prohibited technology list to shill.
  const std::string prohibited_list =
      base::JoinString(prohibited_technologies, ",");

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:22: #include
"chromeos/network/network_handler_callbacks.h"
On 2015/11/11 18:07:31, stevenjb wrote:
> Not needed

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:164: technology);
On 2015/11/11 18:07:31, stevenjb wrote:
> Align

Not modified.
Actually, git cl format made it like this....
If I aligns it, presubmit gives warning to ask me to run git cl format chromeos.
And then clang-format just make it like this again..

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:186: // Remove technologies from the
other lists.
On 2015/11/11 18:07:31, stevenjb wrote:
> Update / move comment

Done.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:197: iter_prohibited_technology++;
On 2015/11/11 18:07:31, stevenjb wrote:
> {}

189-197 no longer used.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:198: }
On 2015/11/11 18:07:31, stevenjb wrote:
> Why not just clear prohibited_technologies_ and replace it with
> prohibited_technologies?

Done.
But is it possible trying SetTechnologyEnabled being called in another thread
while manipulating prohibited_technologies_ here?

[stevenjb, 2015-11-12 18:20:38]

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:164: technology);
On 2015/11/12 10:23:01, fqj wrote:
> On 2015/11/11 18:07:31, stevenjb wrote:
> > Align
> 
> Not modified.
> Actually, git cl format made it like this....
> If I aligns it, presubmit gives warning to ask me to run git cl format
chromeos.
> And then clang-format just make it like this again..

Sorry, you're right, didn't notice there was a + on the prev line.

https://codereview.chromium.org/1431563005/diff/100001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:198: }
On 2015/11/12 10:23:01, fqj wrote:
> On 2015/11/11 18:07:31, stevenjb wrote:
> > Why not just clear prohibited_technologies_ and replace it with
> > prohibited_technologies?
> 
> Done.
> But is it possible trying SetTechnologyEnabled being called in another thread
> while manipulating prohibited_technologies_ here?

No. DBus calls must all be made from the UI thread, so it is important that none
of this code be called from any other thread. (There are checks in lower level
code to catch that).

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:14:
ProhibitedTechnologiesHandler::ProhibitedTechnologiesHandler(){};
' ' before {} (and generally run 'git cl format')

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:63: // Build up prohibited
network type list and saves it for furthur use when
s/saves/save/

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:85: enable_ethernet = false;
Why not just return here? Then you can remove the 'else' also.

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:86: } else {
nit: Add a comment that this is done to ensure that prohibited technologies are
cleared before the user policy is applied. It would be nice to only do this when
necessary (.e.g in Init()), but I geuss there is no harm in doing this more
frequently.

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler.h (right):

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.h:52: bool user_policy_applied_
= false;
Add a blank line before DISALLOW...

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/shill...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:185:
prohibited_technologies_.insert(technology);
prohibited_technologies_.insert(prohibited_technologies.begin(),
prohibited_technologies.end());

[fqj, 2015-11-13 13:53:49]

PTAL.

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:14:
ProhibitedTechnologiesHandler::ProhibitedTechnologiesHandler(){};
On 2015/11/12 18:20:38, stevenjb wrote:
> ' ' before {} (and generally run 'git cl format')

Done.

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:63: // Build up prohibited
network type list and saves it for furthur use when
On 2015/11/12 18:20:38, stevenjb wrote:
> s/saves/save/

Done.

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:85: enable_ethernet = false;
On 2015/11/12 18:20:38, stevenjb wrote:
> Why not just return here? Then you can remove the 'else' also.

Done with "return"

I don't think else can be removed;
The return only returns on Ethernet is prohibited (no need to enable ethernet
back)
but else here means no technologies are prohibited.

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:86: } else {
On 2015/11/12 18:20:38, stevenjb wrote:
> nit: Add a comment that this is done to ensure that prohibited technologies
are
> cleared before the user policy is applied. It would be nice to only do this
when
> necessary (.e.g in Init()), but I geuss there is no harm in doing this more
> frequently.

Done.

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler.h (right):

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.h:52: bool user_policy_applied_
= false;
On 2015/11/12 18:20:38, stevenjb wrote:
> Add a blank line before DISALLOW...

Done.

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/shill...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/shill...
chromeos/network/shill_property_handler.cc:185:
prohibited_technologies_.insert(technology);
On 2015/11/12 18:20:38, stevenjb wrote:
> prohibited_technologies_.insert(prohibited_technologies.begin(),
> prohibited_technologies.end());

Done.

[stevenjb, 2015-11-13 18:26:56]

lgtm

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
File chromeos/network/prohibited_technologies_handler.cc (right):

https://codereview.chromium.org/1431563005/diff/200001/chromeos/network/prohi...
chromeos/network/prohibited_technologies_handler.cc:85: enable_ethernet = false;
On 2015/11/13 13:53:49, fqj wrote:
> On 2015/11/12 18:20:38, stevenjb wrote:
> > Why not just return here? Then you can remove the 'else' also.
> 
> Done with "return"
> 
> I don't think else can be removed;
> The return only returns on Ethernet is prohibited (no need to enable ethernet
> back)
> but else here means no technologies are prohibited.

Right again, sorry, missed the nested if. Too many code reviews of late.

[fqj, 2015-11-13 19:37:47]

The CQ bit was checked by fqj@chromium.org

[fqj, 2015-11-13 19:37:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1431563005/#ps280001
(title: " ")

[commit-bot: I haz the power, 2015-11-13 19:38:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1431563005/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1431563005/280001

[commit-bot: I haz the power, 2015-11-13 20:13:07]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-11-13 20:14:29]

Patchset 15 (id:??) landed as
https://crrev.com/fee5067aaad72d7b0525d23d5f6eb2500831feab
Cr-Commit-Position: refs/heads/master@{#359602}