This requires policy fetch before locking install attributes, caching that policy in authpolicyd and committing it to session manager only after install attributes have been locked.

How about changing Session Manager to allow storage of unsigned policy when install attributes are not fixed yet, but prevent retrieval when policy is unsigned and install attributes are not fixed to AD yet? This way, the flow could be matching the CDM case. Catch: Would have to make sure we retrieve policy from Session Manager after fixing install attributes.

+Mattias.
 
Hey Mattias, could you take a look at Comment 6? Thanks!

I don't think the suggestion in comment #6 is a good idea as it muddles the "unsigned policy only for chromad devices" property. I'd prefer the approach in comment #1, and I think that's also the solution that is more robust in the long term.

Let me also add a bit of experience from the early days of the Chrome OS enterprise effort: IIRC, the cloud management enrollment code used to lock the device also before policy fetching, and this caused issues as well. There was a steady influx of complaints of people getting into weird states because we did the enrollment steps in more or less arbitrary order, and the intermediate states weren't well defined. The solution was to change the enrollment code to do all preparation and validation work up front, and only commit the changes to the device once everything looked good. That eliminated unrecoverable weird device states for the most part and fixed the problem once and for all.

The thing is we can fetch the policy before locking the device. But we can't store it before locking - session managed does not accept unsigned policies before locking the device. So if we lock device and then for some reason fail to store policies into the session manager it would also leave device in a bad state.
I guess we could assume that it would be really rare case.

We could do two refresh device policy calls, one before locking and one after. The first call would return some error indicating that the device isn't locked yet, so store failed, but authpolicy will cache the policy internally. Then on the second call authpolicy would send the cached policy to session manager. I think this is much cleaner than fetching policy from domain join as discussed earlier. WDYT?

Issue 778788 has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/9b6a22544458fc178095b3e9be90edfc6b0aa7ab

commit 9b6a22544458fc178095b3e9be90edfc6b0aa7ab
Author: Roman Sorokin <rsorokin@chromium.org>
Date: Sat Nov 25 04:31:24 2017

system_api: Add new value and protos for authpolicy.

Adds ERROR_CACHE_DEVICE_POLICY: needed to report to Chrome that device
policy cached on authpolicy side.

Adds protos for D-Bus function inputs. Needed to switch D-Bus calls to
protobufs inputs.

BUG= chromium:684679 , chromium:782695 
TEST=none

Change-Id: I35855edc781ad373007a8ef07e55f1e9f02f9016
Reviewed-on: https://chromium-review.googlesource.com/771630
Commit-Ready: Roman Sorokin <rsorokin@chromium.org>
Tested-by: Roman Sorokin <rsorokin@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Roman Sorokin <rsorokin@chromium.org>

[modify] https://crrev.com/9b6a22544458fc178095b3e9be90edfc6b0aa7ab/dbus/authpolicy/active_directory_info.proto

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

commit 6c3600a8920b59899df32cc82e68efce7a30e377
Author: Roman Sorokin <rsorokin@chromium.org>
Date: Sat Nov 25 17:20:46 2017

authpolicy: Cache device policy when device is not locked.

It will use it on the next RefreshDevicePolicy call.

Authpolicyd can not store policy into Session Manager before device
is locked. However locking the device and failure of getting policy after
that did lead to inconsistent state of the device.
To prevent that authpolicyd now gets policy before locking the device,
caches it and stores it after locking.

CQ-DEPEND=CL:771630,CL:774741
BUG= chromium:684679 
TEST=manual

Change-Id: Ifc2a0e853b9eb18e32f4563850cf6edb4c640ec9
Reviewed-on: https://chromium-review.googlesource.com/771710
Commit-Ready: Roman Sorokin <rsorokin@chromium.org>
Tested-by: Roman Sorokin <rsorokin@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Roman Sorokin <rsorokin@chromium.org>

[modify] https://crrev.com/6c3600a8920b59899df32cc82e68efce7a30e377/authpolicy/authpolicy.cc
[modify] https://crrev.com/6c3600a8920b59899df32cc82e68efce7a30e377/authpolicy/authpolicy.h
[modify] https://crrev.com/6c3600a8920b59899df32cc82e68efce7a30e377/authpolicy/authpolicy.gyp
[modify] https://crrev.com/6c3600a8920b59899df32cc82e68efce7a30e377/authpolicy/authpolicy_main.cc
[modify] https://crrev.com/6c3600a8920b59899df32cc82e68efce7a30e377/authpolicy/authpolicy_unittest.cc

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

commit f377998aac728c4f6122ec47a540ede929fbfe05
Author: Roman Sorokin <rsorokin@chromium.org>
Date: Sat Nov 25 20:04:39 2017

Roll src/third_party/cros_system_api/ e46ef54fe..9b6a22544 (1 commit)

    https://chromium.googlesource.com/chromiumos/platform/system_api.git/+log/e46ef54fea48..9b6a22544458

    $ git log e46ef54fe..9b6a22544 --date=short --no-merges --format='%ad %ae %s'
    2017-11-15 rsorokin system_api: Add new value and protos for authpolicy.


    Created with:
      roll-dep src/third_party/cros_system_api
BUG= chromium:684679 , chromium:782695 

Change-Id: I7ed423a228670bc2673559ae0a908821c80e29fb
Reviewed-on: https://chromium-review.googlesource.com/789850
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Commit-Queue: Roman Sorokin <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519186}
[modify] https://crrev.com/f377998aac728c4f6122ec47a540ede929fbfe05/DEPS

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

commit 561fd75da1f4e988250fe01ca33e9ebbbc49114c
Author: Roman Sorokin <rsorokin@chromium.org>
Date: Wed Nov 29 01:25:44 2017

Add new enum values for AuthPolicyErrorType

BUG= chromium:684679 

Change-Id: If9a0ef6666b41e4f214a6334bfc6e4c3f5d84a3d
Reviewed-on: https://chromium-review.googlesource.com/793811
Commit-Queue: Roman Sorokin <rsorokin@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519945}
[modify] https://crrev.com/561fd75da1f4e988250fe01ca33e9ebbbc49114c/tools/metrics/histograms/enums.xml

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

commit fed6482eeff38130f4858cd31307682282e0a142
Author: Roman Sorokin <rsorokin@chromium.org>
Date: Wed Nov 29 11:43:48 2017

Add AuthPolicyLoginHelper::LockDeviceActiveDirectoryForTesting

The function sets install attributes for Active Directory managed
devices.
Changes Active Directory tests to use that function. That way install
attributes consistent between browser and D-Bus fake clients.

BUG=chromium:789120, chromium:684679 

Change-Id: Ic441c7ecc74b5abd06b346157aa044e457fd66c2
Reviewed-on: https://chromium-review.googlesource.com/793170
Commit-Queue: Roman Sorokin <rsorokin@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520076}
[modify] https://crrev.com/fed6482eeff38130f4858cd31307682282e0a142/chrome/browser/chromeos/login/active_directory_login_browsertest.cc
[modify] https://crrev.com/fed6482eeff38130f4858cd31307682282e0a142/chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
[modify] https://crrev.com/fed6482eeff38130f4858cd31307682282e0a142/chrome/browser/chromeos/policy/device_policy_cros_browser_test.cc
[modify] https://crrev.com/fed6482eeff38130f4858cd31307682282e0a142/chrome/browser/chromeos/settings/install_attributes.cc
[modify] https://crrev.com/fed6482eeff38130f4858cd31307682282e0a142/chrome/browser/chromeos/settings/install_attributes.h
[modify] https://crrev.com/fed6482eeff38130f4858cd31307682282e0a142/chromeos/dbus/fake_auth_policy_client_unittest.cc
[modify] https://crrev.com/fed6482eeff38130f4858cd31307682282e0a142/chromeos/login/auth/authpolicy_login_helper.cc
[modify] https://crrev.com/fed6482eeff38130f4858cd31307682282e0a142/chromeos/login/auth/authpolicy_login_helper.h

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

commit 94a94e385a5327eae1b922c8228d9b3d9ee0e4c8
Author: Roman Sorokin <rsorokin@chromium.org>
Date: Wed Nov 29 13:11:21 2017

Chromad: Fetch device policy before locking the device.

Authpolicyd cannot store policy into Session Manager before device
is locked. However locking the device and failure of getting policy
after that did lead to inconsistent state of the device.
To prevent that authpolicyd now gets policy before locking the device,
caches it and stores it after locking the device.

CQ-DEPEND=CL:771710
BUG= chromium:684679 
TEST=manual

Change-Id: Ie6973903b623d7dfdd87aac2c1cbb5fa51132cdd
Reviewed-on: https://chromium-review.googlesource.com/771556
Commit-Queue: Roman Sorokin <rsorokin@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520089}
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chrome/browser/chromeos/login/active_directory_login_browsertest.cc
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chrome/browser/chromeos/policy/active_directory_policy_manager.cc
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chrome/browser/chromeos/policy/active_directory_policy_manager_unittest.cc
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chrome/browser/chromeos/policy/enrollment_handler_chromeos.h
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chromeos/BUILD.gn
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chromeos/dbus/auth_policy_client.cc
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chromeos/dbus/auth_policy_client.h
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chromeos/dbus/fake_auth_policy_client.cc
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chromeos/dbus/fake_auth_policy_client_unittest.cc
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chromeos/login/auth/authpolicy_login_helper.cc
[modify] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chromeos/login/auth/authpolicy_login_helper.h
[add] https://crrev.com/94a94e385a5327eae1b922c8228d9b3d9ee0e4c8/chromeos/login/auth/authpolicy_login_helper_unittest.cc

 Issue 688394  has been merged into this issue.

Verified fixed

Sumo Chrome OS 10451.0.0, 66.0.3358.0