New issue
Advanced search Search tips

Issue 722799 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jun 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Fix code expecting "has_request_token() => is enterprise managed"

Project Member Reported by ljusten@chromium.org, May 16 2017

Issue description

A few instances in code expect that the existence of a request token (DM token) in enterprise_management::PolicyData implies that the device is enterprise managed. This is currently not true for Chromad, where the device is AD enterprise managed, but there's no request token.

Can we just assign a fake request token in authpolicyd?

https://cs.chromium.org/search/?q=%5Cbhas_request_token%5Cb&type=cs
https://cs.corp.google.com/search/?q=has_request_token+package:%5Echromeos_public$&m=100&type=cs

 
Please also update DevicePolicyImpl::IsEnterpriseManaged() that I'm currently adding.
Presence of the request token is a legacy way of checking whether a device or user is managed or not. The new way is the |management_mode| field. Note that you'll have to take into account backwards compatibility, i.e. if management_mode is not present, fall back to the token presence check. However, for chromad you can probably make sure that management_mode is set adequately (maybe you even want to add another enum value for this case?).

Comment 3 by tnagel@chromium.org, May 22 2017

From reading the Chrome InstallAttributes code [1], in case "enterprise.mode" isn't present, it's possible to fallback to "enterprise.owned".

[1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/settings/install_attributes.cc?rcl=863dc7c7934fb53d57d29173a5c99a47f3a7bac6&l=509

Comment 4 by tnagel@chromium.org, May 22 2017

Cc: mnissler@chromium.org
FTR:
"enterprise.mode" was added in M19, CL [1]
"enterprise.owned" was added in M12, CL [2]

Discounting mario, alex and zbg which are already EOL, there's only pre-19 FSIs for lumpy and stumpy.  Thus we need to wait for lumpy/stumpy EOL before "enterprise.owned" can be removed.

[1] https://chromiumcodereview.appspot.com/9403010
[2] https://codereview.chromium.org/6821075

Comment 5 by tnagel@chromium.org, May 24 2017

Cc: -mnissler@chromium.org
Moved enterprise.owned deprecation to a separate bug: issue 725918.  Also moving cc:mnissler there.

Comment 6 by tnagel@chromium.org, May 24 2017

Probably we should go through the PolicyFetchResponse and PolicyData protos and update the docs wherever AD behaviour differs from cloud management.

FTR: authpolicy assembles these protos in AuthPolicy::StorePolicy() and leaves almost all of the fields unset.

Comment 7 by tnagel@chromium.org, May 24 2017

In the Chrome codebase, I could only find one use of has_request_token() that seemed problematic.  (The others either looked legit or seemed confined to cloud-only code parts.)

CL: https://codereview.chromium.org/2902183002/

Comment 8 by tnagel@chromium.org, May 24 2017

Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, May 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/c3667249b09c5407d026720f84dcbbae0784b93c

commit c3667249b09c5407d026720f84dcbbae0784b93c
Author: Thiemo Nagel <tnagel@chromium.org>
Date: Tue May 30 23:29:03 2017

login: Fix calculation of management state

Use management_mode to determine whether device is managed and only
fall back to DM token in case the former doesn't exist.  This is
required to properly support Chromad, but should have no effect on
other types of management (including local ownership).

BUG= chromium:722799 
TEST=unit test added

Change-Id: I8f081c6779cf9a5c38de0e11e59524d074738419
Reviewed-on: https://chromium-review.googlesource.com/518015
Commit-Ready: Thiemo Nagel <tnagel@chromium.org>
Tested-by: Thiemo Nagel <tnagel@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/c3667249b09c5407d026720f84dcbbae0784b93c/login_manager/device_policy_service.cc
[modify] https://crrev.com/c3667249b09c5407d026720f84dcbbae0784b93c/login_manager/device_policy_service.h
[modify] https://crrev.com/c3667249b09c5407d026720f84dcbbae0784b93c/login_manager/device_policy_service_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, May 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/external/libbrillo/+/d710b3bbaf7a8253830cbe9d2e720ac8bde16251

commit d710b3bbaf7a8253830cbe9d2e720ac8bde16251
Author: Thiemo Nagel <tnagel@chromium.org>
Date: Wed May 31 19:38:39 2017

libbrillo: Add DevicePolicy::IsEnterpriseManaged()

Use management_mode to determine whether device is managed and only
fall back to DM token in case the former doesn't exist.  This is
required to properly support Chromad, but should have no effect on
other types of management (including local ownership).

BUG= chromium:722799 
TEST=unit tests added

Change-Id: Ifa8bc6bf616dc23eb767c97934fe4cdb77a3451a
Reviewed-on: https://chromium-review.googlesource.com/518045
Commit-Ready: Thiemo Nagel <tnagel@chromium.org>
Tested-by: Thiemo Nagel <tnagel@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[add] https://crrev.com/d710b3bbaf7a8253830cbe9d2e720ac8bde16251/policy/tests/device_policy_impl_unittest.cc
[modify] https://crrev.com/d710b3bbaf7a8253830cbe9d2e720ac8bde16251/policy/mock_device_policy.h
[modify] https://crrev.com/d710b3bbaf7a8253830cbe9d2e720ac8bde16251/policy/device_policy_impl.h
[modify] https://crrev.com/d710b3bbaf7a8253830cbe9d2e720ac8bde16251/libbrillo.gypi
[modify] https://crrev.com/d710b3bbaf7a8253830cbe9d2e720ac8bde16251/policy/device_policy.h
[modify] https://crrev.com/d710b3bbaf7a8253830cbe9d2e720ac8bde16251/policy/device_policy_impl.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92db6b2c14f28d8339590ea4c6a0186d9bbf08c5

commit 92db6b2c14f28d8339590ea4c6a0186d9bbf08c5
Author: tnagel <tnagel@chromium.org>
Date: Thu Jun 01 09:47:34 2017

Improve determination of managed state in DeviceSettingsProvider

Use PolicyData::management_mode to determine whether the device has a
local owner and only fall back to DM token in case management_mode is
unset.  That way, the correct determination is guaranteed for both
cloud and Active Directory management.  (Currently, the code is not
broken because AD policy doesn't include a user name, but that might
change in the future.)

Also update the PolicyData::management_mode documentation to include
Active Directory.

BUG= 722799 

Review-Url: https://codereview.chromium.org/2902183002
Cr-Commit-Position: refs/heads/master@{#476239}

[modify] https://crrev.com/92db6b2c14f28d8339590ea4c6a0186d9bbf08c5/chrome/browser/chromeos/settings/device_settings_provider.cc
[modify] https://crrev.com/92db6b2c14f28d8339590ea4c6a0186d9bbf08c5/components/policy/proto/device_management_backend.proto

Labels: M-61
Status: Fixed (was: Started)
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/c7b81397e440e6f0e6e9748ace1033f8fc2df205

commit c7b81397e440e6f0e6e9748ace1033f8fc2df205
Author: Thiemo Nagel <tnagel@chromium.org>
Date: Fri Jun 02 12:06:31 2017

login: Source code cosmetics

Reorder methods to have the two static ones together.  No functional
change.

BUG= chromium:722799 
TEST=existing tests

Change-Id: I6a8bf223ac636e2ef78318d630f8440e3f797e61
Reviewed-on: https://chromium-review.googlesource.com/519325
Commit-Ready: Thiemo Nagel <tnagel@chromium.org>
Tested-by: Thiemo Nagel <tnagel@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/c7b81397e440e6f0e6e9748ace1033f8fc2df205/login_manager/device_policy_service.cc
[modify] https://crrev.com/c7b81397e440e6f0e6e9748ace1033f8fc2df205/login_manager/device_policy_service.h

Status: Verified (was: Fixed)
bulk Verify of older or not-user-facing Chromad bugs

Sign in to add a comment