Report logged in users on enterprise managed devices.

Report logged in users on enterprise managed devices.

BUG=308664
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231716

[Joao da Silva, 2013-10-14 14:59:44]

Straightforward impl and nice test coverage :-) This just needs a bug with a
privacy and security review.

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/mock_user_manager.cc (right):

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/mock_user_manager.cc:72:
user_list_.insert(user_list_.begin(), User::CreateRegularUser(email));
Why doesn't AddUser() add at the back and use push_back here?

Another option is to document that user_list_ is in reverse order for this
class, and use push_back here (and return back() in GetLoggedInUser())

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/mock_user_manager.cc:79: user_list_.clear();
This can be just

  STLDeleteElements(&user_list_);

from base/stl_util.h

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:145: class
FakeEnterpriseInstallAttributes
chrome/browser/chromeos/policy/stub_enterprise_install_attributes.h can handle
this

https://codereview.chromium.org/26770002/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/26770002/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:443:
install_attributes_.reset(attributes);
The previous instance has already been passed to other components in the
constructor, so this can easily lead to crashy tests.

Use a global static like g_testing_provider instead, and use it in the ctor
instead:

  EnterpriseInstallAttributes* install_attributes_;
  scoped_ptr<EnterpriseInstallAttributes> owned_install_attributes_;

And then in ctor:

  if (g_install_attributes) {
    install_attributes_ = g_install_attributes;
  } else {
    owned_install_attributes_.reset( new ... );
    install_attributes_ = owned_install_attributes_.get();
  }

Note that the global must be set before the browser is created, which is when
the BrowserPolicyConnector gets created. In browser_tests this can be done in
SetUpInProcessBrowserTestFixture().

[Steve Condie, 2013-10-14 19:29:47]

Added Dave for changes to MockUserManager and new setting.

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/mock_user_manager.cc (right):

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/mock_user_manager.cc:72:
user_list_.insert(user_list_.begin(), User::CreateRegularUser(email));
On 2013/10/14 14:59:45, Joao da Silva wrote:
> Why doesn't AddUser() add at the back and use push_back here?
> 
> Another option is to document that user_list_ is in reverse order for this
> class, and use push_back here (and return back() in GetLoggedInUser())

Done.

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/mock_user_manager.cc:79: user_list_.clear();
On 2013/10/14 14:59:45, Joao da Silva wrote:
> This can be just
> 
>   STLDeleteElements(&user_list_);
> 
> from base/stl_util.h

The destructor is protected. This follows the code in
UserManagerImpl::~UserManagerImpl. Added a similar comment here.

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/26770002/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:145: class
FakeEnterpriseInstallAttributes
On 2013/10/14 14:59:45, Joao da Silva wrote:
> chrome/browser/chromeos/policy/stub_enterprise_install_attributes.h can handle
> this

Thanks, didn't see that.

https://codereview.chromium.org/26770002/diff/1/chrome/browser/policy/browser...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/26770002/diff/1/chrome/browser/policy/browser...
chrome/browser/policy/browser_policy_connector.cc:443:
install_attributes_.reset(attributes);
On 2013/10/14 14:59:45, Joao da Silva wrote:
> The previous instance has already been passed to other components in the
> constructor, so this can easily lead to crashy tests.
> 
> Use a global static like g_testing_provider instead, and use it in the ctor
> instead:
> 
>   EnterpriseInstallAttributes* install_attributes_;
>   scoped_ptr<EnterpriseInstallAttributes> owned_install_attributes_;
> 
> And then in ctor:
> 
>   if (g_install_attributes) {
>     install_attributes_ = g_install_attributes;
>   } else {
>     owned_install_attributes_.reset( new ... );
>     install_attributes_ = owned_install_attributes_.get();
>   }
> 
> Note that the global must be set before the browser is created, which is when
> the BrowserPolicyConnector gets created. In browser_tests this can be done in
> SetUpInProcessBrowserTestFixture().

Done.

[Joao da Silva, 2013-10-15 09:13:47]

lgtm after moving install_attributes_for_testing_

Have you pinged Sumit about this change?

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:192: #endif
Define this next to g_testing_provider instead, and not as a member of BPC.

Also, no space in defined(OS_CHROMEOS)

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:218: new
EnterpriseInstallAttributes(cryptohome_client));
It's not entirely clear if some tests enter here or not. Better to test
|install_attributes_| before resetting here.

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.h:179: static
EnterpriseInstallAttributes* install_attributes_for_testing_;
Don't declare this here; define this as a global in the cc file, next to
g_testing_provider

[Steve Condie, 2013-10-15 23:47:01]

Yes, pinged Sumit; haven't heard back yet.

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.cc (right):

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:192: #endif
On 2013/10/15 09:13:47, Joao da Silva wrote:
> Define this next to g_testing_provider instead, and not as a member of BPC.
> 
> Also, no space in defined(OS_CHROMEOS)

Done.

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.cc:218: new
EnterpriseInstallAttributes(cryptohome_client));
On 2013/10/15 09:13:47, Joao da Silva wrote:
> It's not entirely clear if some tests enter here or not. Better to test
> |install_attributes_| before resetting here.

Done.

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
File chrome/browser/policy/browser_policy_connector.h (right):

https://codereview.chromium.org/26770002/diff/28001/chrome/browser/policy/bro...
chrome/browser/policy/browser_policy_connector.h:179: static
EnterpriseInstallAttributes* install_attributes_for_testing_;
On 2013/10/15 09:13:47, Joao da Silva wrote:
> Don't declare this here; define this as a global in the cc file, next to
> g_testing_provider

Done.

[Steve Condie, 2013-10-16 17:40:40]

David, would you be a good person to review this? You're in the owners of both
chromeos/ and chrome/browser/chromeos/login/.

[Steve Condie, 2013-10-17 18:50:16]

Added reviewer Dmitri for MockUserManager update to allow multiple users to be
added to the user list.

Added reviewers Julian and Steven for new ChromeOS setting to control whether
device users are reported.

[stevenjb, 2013-10-17 19:32:48]

chromeos/settings lgtm, just one comment.

https://codereview.chromium.org/26770002/diff/52001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
(right):

https://codereview.chromium.org/26770002/diff/52001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/device_status_collector_browsertest.cc:619:
EXPECT_EQ(5, status_.user_size());
nit: Maybe add a getter to DeviceStatusCollector to return the max number of
users and test against that. Also comment above or use a loop to add N+1 users
(and maybe test unmanaged separately)

[Dmitry Polukhin, 2013-10-21 18:28:13]

LGTM for chrome/browser/chromeos/login/*

[Steve Condie, 2013-10-29 18:15:15]

Added Ilya since I added new policies to histograms.xml per presubmit check.
Ilya I manually added the values; is there a tool to automatically update
histograms.xml from policy_templates.json?

[Ilya Sherman, 2013-10-29 21:17:21]

On 2013/10/29 18:15:15, stepco wrote:
> Added Ilya since I added new policies to histograms.xml per presubmit check.
> Ilya I manually added the values; is there a tool to automatically update
> histograms.xml from policy_templates.json?

Please run update_policies.py to update this histogram.  Thanks.

[Steve Condie, 2013-10-29 21:30:26]

On 2013/10/29 21:17:21, Ilya Sherman wrote:
> On 2013/10/29 18:15:15, stepco wrote:
> > Added Ilya since I added new policies to histograms.xml per presubmit check.
> > Ilya I manually added the values; is there a tool to automatically update
> > histograms.xml from policy_templates.json?
> 
> Please run update_policies.py to update this histogram.  Thanks.

Done

[Ilya Sherman, 2013-10-29 21:33:33]

histograms.xml lgtm, thanks.

[commit-bot: I haz the power, 2013-10-29 22:33:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stepco@chromium.org/26770002/152001

[commit-bot: I haz the power, 2013-10-30 03:05:03]

Change committed as 231716