Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2954293002:
Chromad: Prevent session from starting without policy (Closed)
|
Created:
3 years, 5 months ago by Thiemo Nagel Modified:
3 years, 5 months ago CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionInterface and implementation are modeled after
UserCloudPolicyManagerChromeOS. Also added some slight
modifications/explanations to the latter.
BUG=732801
TEST=added unit tests
Review-Url: https://codereview.chromium.org/2954293002
Cr-Commit-Position: refs/heads/master@{#487791}
Committed: https://chromium.googlesource.com/chromium/src/+/cab862ac8252d68584eaf94f9b19e639f03200d7
Patch Set 1 : Integrate session exit into tests #Patch Set 2 : Comment fixes #
Total comments: 14
Patch Set 3 : Polish #Patch Set 4 : Fix initialization complete for component policy #
Total comments: 10
Patch Set 5 : Address nits #
Total comments: 10
Patch Set 6 : Drop |wait_for_policy_fetch| and minor improvements #Patch Set 7 : Move MockAuthPolicyClient into unittest #Messages
Total messages: 72 (54 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== WIP -- harden AD policy enforcment BUG= ========== to ========== WIP -- harden AD policy enforcment BUG=732801 ==========
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP -- harden AD policy enforcment BUG=732801 ========== to ========== Modeled after UserCloudPolicyManagerChromeOS and added some slight modifications/explanations to the latter. BUG=732801 TEST=added unit tests ==========
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
tnagel@chromium.org changed reviewers: + emaxx@chromium.org
Hey Maksim, may I ask you to take a look? This should be equivalent to the work that Drew and you have been doing to prevent the session from starting when cloud policy is missing. Thank you! Thiemo
On 2017/07/11 14:19:39, Thiemo Nagel wrote: > Hey Maksim, > > may I ask you to take a look? This should be equivalent to the work that Drew > and you have been doing to prevent the session from starting when cloud policy > is missing. > > Thank you! > Thiemo Will try to take a look by tomorrow's EOD, sorry.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Will try to take a look by tomorrow's EOD, sorry. Thank you, np!
Description was changed from ========== Modeled after UserCloudPolicyManagerChromeOS and added some slight modifications/explanations to the latter. BUG=732801 TEST=added unit tests ========== to ========== Interface and implementation are modeled after UserCloudPolicyManagerChromeOS. Also added some slight modifications/explanations to the latter. BUG=732801 TEST=added unit tests ==========
Haven't read the tests yet, but I'm a bit concerned by the complexity around state transitions in the ActiveDirectoryPolicyManager (please also see the comments below). Could maybe some brainstorming help to find a better way to encode all those? https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:86: return false; Not sure I understand this change: now the component policies are always reported as "uninitialized", while previously they were always "initialized" - is that an expected change? https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:139: base::Unretained(this))); nit: Ideally we'd have this documented why Unretained is safe here (although I know that this was copied from the existing class which doesn't have this). https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:207: if (!store_->has_policy()) { I'm quite confused about complexity of the state machine in this class. However, I'm not ready to give any specific advice here. But what about this case - isn't there a race condition: 1. Init(). (This calls into store_->Load().) 2. OnPolicyFetched(true). (This calls into store_->Load() again.) 3. OnStoreLoaded(), which is called as the result of *first* store_->Load(). (This calls into CancelWaitForPolicy().) On step #3, if store_ is unlucky to not have loaded the policy fetch result in time, the condition at this line will lead to the user being signed out. Or am I missing something? In any case, I'd be glad if some simpler way to handle all those state transitions could be found... https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:84: // Called when |policy_fetch_timeout_| times out, to cancel the blocking wait nit: s/policy_fetch_timeout_/initial_policy_timeout_/ https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:91: void CancelWaitForPolicy(bool success); nit: Maybe add s/ForPolicy/ForInitialPolicy/ (or even ForInitialPolicyFetch), for clarity? https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:111: base::Timer initial_policy_timeout_{false /* retain_user_task */, nit: If you're not opposed to too long names, I'd prefer "initial_policy_fetch_timeout_". This would clarify that it's not only about a freshly created profile, but is about the first *fetch* in the current session. https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h (right): https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h:169: base::Timer policy_fetch_timeout_{false /* retain_user_task */, (just note) Thanks for doing this cleanup!
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've addressed your comments and added/improved docs. Could you please take another look? https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:86: return false; On 2017/07/12 20:18:19, emaxx wrote: > Not sure I understand this change: now the component policies are always > reported as "uninitialized", while previously they were always "initialized" - > is that an expected change? Since we don't support component policy for Chromad (yet), I thought returning false would make more sense. But maybe it's a bad idea? https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:139: base::Unretained(this))); On 2017/07/12 20:18:19, emaxx wrote: > nit: Ideally we'd have this documented why Unretained is safe here (although I > know that this was copied from the existing class which doesn't have this). Good point. It used to be safe because the timer was the last member of the class but that isn't true anymore... :-O Probably better to use a weak pointer. https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:207: if (!store_->has_policy()) { On 2017/07/12 20:18:20, emaxx wrote: > I'm quite confused about complexity of the state machine in this class. > However, I'm not ready to give any specific advice here. > > But what about this case - isn't there a race condition: > 1. Init(). (This calls into store_->Load().) > 2. OnPolicyFetched(true). (This calls into store_->Load() again.) > 3. OnStoreLoaded(), which is called as the result of *first* store_->Load(). > (This calls into CancelWaitForPolicy().) > On step #3, if store_ is unlucky to not have loaded the policy fetch result in > time, the condition at this line will lead to the user being signed out. > Or am I missing something? > > In any case, I'd be glad if some simpler way to handle all those state > transitions could be found... After a lot of offline discussion: It's hard to simplify the logic without sacrificing performance (i.e. de-parallelizing the code). store_->Load() cancels the previous load operation, thus there is no race condition. https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:84: // Called when |policy_fetch_timeout_| times out, to cancel the blocking wait On 2017/07/12 20:18:20, emaxx wrote: > nit: s/policy_fetch_timeout_/initial_policy_timeout_/ Done. https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:91: void CancelWaitForPolicy(bool success); On 2017/07/12 20:18:20, emaxx wrote: > nit: Maybe add s/ForPolicy/ForInitialPolicy/ (or even ForInitialPolicyFetch), > for clarity? Thanks. Done. I'm not adding Fetch because a Load can also be sufficient under some circumstances. https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:111: base::Timer initial_policy_timeout_{false /* retain_user_task */, On 2017/07/12 20:18:20, emaxx wrote: > nit: If you're not opposed to too long names, I'd prefer > "initial_policy_fetch_timeout_". This would clarify that it's not only about a > freshly created profile, but is about the first *fetch* in the current session. Same as above, I think writing "fetch" hides the fact that "load" can be sufficient.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:86: return false; > Since we don't support component policy for Chromad (yet), I thought returning > false would make more sense. But maybe it's a bad idea? I've changed it back.
LGTM % nits https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:98: CancelWaitForInitialPolicy(fetch_ever_succeeded_ /* success */); nit: What about a comment like this: // Stop waiting for the initial policy load - if the initial fetch // succeeded, then its result should have already become loaded by // this point (as the fetch notification cancels all running load // operations). https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:132: DCHECK(!(account_id == EmptyAccountId() && wait_for_policy_fetch)); nit: BTW, why is there such a restriction? Could you maybe highlight a reason for that in a short comment? https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:65: // successful policy fetch from the server or |initial_policy_fetch_timeout| nit: The description seems to be a bit inaccurate: the initialization may happen even after the failed fetch - in that case, the store load is awaited. https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:90: // Cancels waiting for the initial policy fetch/load and flags the nit: Worth explaning that it doesn't do this unconditionally - in case of the blocking fetch the error to obtain the policy will result in the |exit_session| call. https://codereview.chromium.org/2954293002/diff/180001/chromeos/dbus/mock_aut... File chromeos/dbus/mock_auth_policy_client.h (right): https://codereview.chromium.org/2954293002/diff/180001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:32: NOTREACHED(); nit: Did you consider NOTIMPLEMENTED()?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Many thanks! I've addressed the nits. Would you like to take another look? https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:98: CancelWaitForInitialPolicy(fetch_ever_succeeded_ /* success */); On 2017/07/14 13:56:40, emaxx wrote: > nit: What about a comment like this: > // Stop waiting for the initial policy load - if the initial fetch > // succeeded, then its result should have already become loaded by > // this point (as the fetch notification cancels all running load > // operations). Done. https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:132: DCHECK(!(account_id == EmptyAccountId() && wait_for_policy_fetch)); On 2017/07/14 13:56:40, emaxx wrote: > nit: BTW, why is there such a restriction? Could you maybe highlight a reason > for that in a short comment? Done. https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right): https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:65: // successful policy fetch from the server or |initial_policy_fetch_timeout| On 2017/07/14 13:56:40, emaxx wrote: > nit: The description seems to be a bit inaccurate: the initialization may happen > even after the failed fetch - in that case, the store load is awaited. Thanks. I've added more text, hopefully it's clearer now. https://codereview.chromium.org/2954293002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.h:90: // Cancels waiting for the initial policy fetch/load and flags the On 2017/07/14 13:56:40, emaxx wrote: > nit: Worth explaning that it doesn't do this unconditionally - in case of the > blocking fetch the error to obtain the policy will result in the |exit_session| > call. Good point. Done. https://codereview.chromium.org/2954293002/diff/180001/chromeos/dbus/mock_aut... File chromeos/dbus/mock_auth_policy_client.h (right): https://codereview.chromium.org/2954293002/diff/180001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:32: NOTREACHED(); On 2017/07/14 13:56:40, emaxx wrote: > nit: Did you consider NOTIMPLEMENTED()? Done.
tnagel@chromium.org changed reviewers: + stevenjb@chromium.org
Hey Steven, may I ask you to take a look at this tiny change in DBusThread Manager? -- I'm adding SetAuthPolicyClient() to DBusThreadManagerSetter. Thank you and kind regards, Thiemo
On 2017/07/17 09:29:35, Thiemo Nagel wrote: > Many thanks! I've addressed the nits. Would you like to take another look? Still LGTM!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2954293002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:137: DCHECK_NE(wait_for_policy_fetch, initial_policy_fetch_timeout.is_zero()); nit: Do we need |wait_for_policy_fetch| as a parameter? I think the code might actually be more clear with one less parameter and wait_for_policy fetch == !initial_policy_fetch_timeout.is_zero(). https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... File chromeos/dbus/mock_auth_policy_client.h (right): https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:30: JoinCallback callback) { override for all of these? https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:31: // Not implemented. Comment not really helpful :) (here and below) https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:33: } This isn't really a mock, it's closer to a Fake. We should either use all MOCK_METHOD or make this FakeAuthPolicyClient.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:220001) has been deleted
Many thanks Steven! Could you please take another look? https://codereview.chromium.org/2954293002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right): https://codereview.chromium.org/2954293002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/policy/active_directory_policy_manager.cc:137: DCHECK_NE(wait_for_policy_fetch, initial_policy_fetch_timeout.is_zero()); On 2017/07/17 16:07:37, stevenjb wrote: > nit: Do we need |wait_for_policy_fetch| as a parameter? I think the code might > actually be more clear with one less parameter and wait_for_policy fetch == > !initial_policy_fetch_timeout.is_zero(). I like that very much. Thanks! https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... File chromeos/dbus/mock_auth_policy_client.h (right): https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:30: JoinCallback callback) { On 2017/07/17 16:07:37, stevenjb wrote: > override for all of these? Done. https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:31: // Not implemented. On 2017/07/17 16:07:37, stevenjb wrote: > Comment not really helpful :) (here and below) Good point. :) That used to be NOTREACHED(). https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:33: } On 2017/07/17 16:07:37, stevenjb wrote: > This isn't really a mock, it's closer to a Fake. We should either use all > MOCK_METHOD or make this FakeAuthPolicyClient. There is already a FakeAuthPolicyClient which is somewhat more heavyweight. The only reason I'm not using MOCK_METHOD is that it doesn't work with (non-copyable) OnceCallbacks afaiu.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... File chromeos/dbus/mock_auth_policy_client.h (right): https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:33: } On 2017/07/18 13:02:18, Thiemo Nagel wrote: > On 2017/07/17 16:07:37, stevenjb wrote: > > This isn't really a mock, it's closer to a Fake. We should either use all > > MOCK_METHOD or make this FakeAuthPolicyClient. > > There is already a FakeAuthPolicyClient which is somewhat more heavyweight. The > only reason I'm not using MOCK_METHOD is that it doesn't work with > (non-copyable) OnceCallbacks afaiu. Hmm. In that case, if this is only used in one test file, I would move this to the test file itself and name it TestAuthPolicyClient. That said, I won't block on this if we need it in multiple places, but having a "Mock" class that isn't really a mock seems confusing to me. Will it work as written with gmock test expectations, etc? (I'm not actually super familiar with gmock).
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! Would like to take another look? (I plan to land tomorrow unless there are further comments.) https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... File chromeos/dbus/mock_auth_policy_client.h (right): https://codereview.chromium.org/2954293002/diff/200001/chromeos/dbus/mock_aut... chromeos/dbus/mock_auth_policy_client.h:33: } On 2017/07/18 15:37:44, stevenjb wrote: > On 2017/07/18 13:02:18, Thiemo Nagel wrote: > > On 2017/07/17 16:07:37, stevenjb wrote: > > > This isn't really a mock, it's closer to a Fake. We should either use all > > > MOCK_METHOD or make this FakeAuthPolicyClient. > > > > There is already a FakeAuthPolicyClient which is somewhat more heavyweight. > The > > only reason I'm not using MOCK_METHOD is that it doesn't work with > > (non-copyable) OnceCallbacks afaiu. > > Hmm. In that case, if this is only used in one test file, I would move this to > the test file itself and name it TestAuthPolicyClient. Thanks, done. > That said, I won't block on this if we need it in multiple places, but having a > "Mock" class that isn't really a mock seems confusing to me. Will it work as > written with gmock test expectations, etc? (I'm not actually super familiar with > gmock). No, it won't.
I do like this better, thanks. lgtm++
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/18 16:06:05, stevenjb wrote: > I do like this better, thanks. lgtm++ Thanks again!
The CQ bit was checked by tnagel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2954293002/#ps260001 (title: "Move MockAuthPolicyClient into unittest")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1500452101965080,
"parent_rev": "9934ccbc242de2a3c5dff837083703065c9926bd", "commit_rev":
"cab862ac8252d68584eaf94f9b19e639f03200d7"}
Message was sent while issue was closed.
Description was changed from ========== Interface and implementation are modeled after UserCloudPolicyManagerChromeOS. Also added some slight modifications/explanations to the latter. BUG=732801 TEST=added unit tests ========== to ========== Interface and implementation are modeled after UserCloudPolicyManagerChromeOS. Also added some slight modifications/explanations to the latter. BUG=732801 TEST=added unit tests Review-Url: https://codereview.chromium.org/2954293002 Cr-Commit-Position: refs/heads/master@{#487791} Committed: https://chromium.googlesource.com/chromium/src/+/cab862ac8252d68584eaf94f9b19... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001) as https://chromium.googlesource.com/chromium/src/+/cab862ac8252d68584eaf94f9b19... |
