Assume that time is synchronized until response from TLSDate arrives

There is a policy "DeviceOffHours" which defines some time intervals
when some policies gets deleted from policy blob. Among them there is a
policy "DeviceGuestModeEnabled". If it's set to false after removing it
from policy blob it should default to "true", thus allowing browsing as
a guest.

Before: There is a button on login screen "Browser as Guest", because
policy "DeviceGuestModeEnabled" defaulted to "true" after deleting it
from policy blob. But when pressing it Chrome restarts and tries to
initialize profile synchronously. But "DeviceOffHours" doesn't get
applied because it's a policy that relies on time, and makes async DBus
call. So DeviceOffHoursController assumes that time is not reliable and
logs out the user.

After: Assume that time is reliable until the response from DBus call
arrived, thus allowing synchronous profile initialization to finish.
After finishing, response from DBus call arrives and corresponding
actions are taken (log user out if time is not synchronized).

User log out flow:
* DeviceOffHoursController::UpdateOffHoursMode               =>
* DeviceOffHoursController::SetOffHoursMode                  =>
* DeviceOffHoursController::OffHoursModeIsChanged            =>
* DeviceSettingsService::Load                                =>
* DeviceSettingsService::HandleCompletedOperation            =>
* DeviceSettingsService::NotifyDeviceSettingsUpdated         =>
* DeviceSettingsProvider::DeviceSettingsUpdated              =>
* DeviceSettingsProvider::UpdateAndProceedStoring            =>
* DeviceSettingsProvider::UpdateFromService                  =>
* DeviceSettingsProvider::UpdateValuesCache                  =>
* DeviceSettingsProvider::DecodePolicies                     =>
* DecodeLoginPolicies (in device_settings_provider.cc)       =>
* DecodeLoginPolicies changes CrosSettings                   =>
* CrosSettings change gets observed by ChromeUserManagerImpl =>
* UserManagerBase::NotifyUsersSignInConstraintsChanged       =>
* UserSessionManager::OnUsersSignInConstraintsChanged        =>
* AttemptUserExit

Test: Set DeviceGuestModeEnabled to "false", Configure "DeviceOffHours"
policy to have an interval from Monday 12:00am till Sunday 11:30pm.
During an Off Hours interval sign in to the device, disable all the
network connections. Exptectation: user will be kicked out as soon
network connections are gone.

Bug: 930567
Change-Id: I4f7fd429ba7cb64cf99a943e1f5e8686801279d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1699667
Commit-Queue: Askar Aitzhan <askaraitzhan@google.com>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682257}

chrome/browser/chromeos/policy/off_hours/device_off_hours_controller.cc

@@ -130,8 +130,11 @@ void DeviceOffHoursController::OffHoursModeIsChanged() const {
 }
 
 void DeviceOffHoursController::UpdateOffHoursMode() {
-  if (off_hours_intervals_.empty() || !network_synchronized_) {
-    if (!network_synchronized_) {
+  // Assume that time is network synchronized if response from dbus call is not
+  // arrived.
+  bool is_time_network_synchronized = network_synchronized_.value_or(true);
+  if (off_hours_intervals_.empty() || !is_time_network_synchronized) {
+    if (!is_time_network_synchronized) {
       VLOG(1) << "The system time isn't network synchronized. OffHours mode is "
                  "unavailable.";
     }

chrome/browser/chromeos/policy/off_hours/device_off_hours_controller.h

@@ -136,8 +136,10 @@ class DeviceOffHoursController : public chromeos::SystemClockClient::Observer,
   // base::DefaultClock.
   base::Clock* clock_;
 
-  // Value is false until the system time is synchronized with network time.
-  bool network_synchronized_ = false;
+  // Value is true if the system time is synchronized with network time, and
+  // false when synchronization failed. Value is not set until the response from
+  // the D-Bus call is not arrived.
+  base::Optional<bool> network_synchronized_;
 
   // Current "OffHours" time intervals.
   std::vector<WeeklyTimeInterval> off_hours_intervals_;

chrome/browser/chromeos/policy/off_hours/device_off_hours_controller_unittest.cc

@@ -108,6 +108,7 @@ class DeviceOffHoursControllerSimpleTest
   void SetUp() override {
     chromeos::DeviceSettingsTestBase::SetUp();
     chromeos::SystemClockClient::InitializeFake();
+    system_clock_client()->SetServiceIsAvailable(false);
 
     device_settings_service_->SetDeviceOffHoursControllerForTesting(
         std::make_unique<policy::off_hours::DeviceOffHoursController>());
@@ -156,6 +157,7 @@ class DeviceOffHoursControllerSimpleTest
 };
 
 TEST_F(DeviceOffHoursControllerSimpleTest, CheckOffHoursUnset) {
+  system_clock_client()->SetServiceIsAvailable(true);
   system_clock_client()->SetNetworkSynchronized(true);
   system_clock_client()->NotifyObserversSystemClockUpdated();
   em::ChromeDeviceSettingsProto& proto(device_policy_->payload());
@@ -172,6 +174,7 @@ TEST_F(DeviceOffHoursControllerSimpleTest, CheckOffHoursUnset) {
 }
 
 TEST_F(DeviceOffHoursControllerSimpleTest, CheckOffHoursModeOff) {
+  system_clock_client()->SetServiceIsAvailable(true);
   system_clock_client()->SetNetworkSynchronized(true);
   system_clock_client()->NotifyObserversSystemClockUpdated();
   em::ChromeDeviceSettingsProto& proto(device_policy_->payload());
@@ -197,6 +200,7 @@ TEST_F(DeviceOffHoursControllerSimpleTest, CheckOffHoursModeOff) {
 }
 
 TEST_F(DeviceOffHoursControllerSimpleTest, CheckOffHoursModeOn) {
+  system_clock_client()->SetServiceIsAvailable(true);
   system_clock_client()->SetNetworkSynchronized(true);
   system_clock_client()->NotifyObserversSystemClockUpdated();
   em::ChromeDeviceSettingsProto& proto(device_policy_->payload());
@@ -220,7 +224,37 @@ TEST_F(DeviceOffHoursControllerSimpleTest, CheckOffHoursModeOn) {
                   .guest_mode_enabled());
 }
 
+TEST_F(DeviceOffHoursControllerSimpleTest,
+       CheckOffHoursEnabledBeforeSystemClockUpdated) {
+  system_clock_client()->SetServiceIsAvailable(false);
+  em::ChromeDeviceSettingsProto& proto(device_policy_->payload());
+  proto.mutable_guest_mode_enabled()->set_guest_mode_enabled(false);
+  UpdateDeviceSettings();
+  int current_day_of_week = ExtractDayOfWeek(base::Time::Now());
+  SetOffHoursPolicyToProto(
+      &proto,
+      OffHoursPolicy(
+          kUtcTimezone,
+          {WeeklyTimeInterval(
+              WeeklyTime(current_day_of_week, 0, 0),
+              WeeklyTime(NextDayOfWeek(current_day_of_week),
+                         TimeDelta::FromHours(10).InMilliseconds(), 0))}));
+  UpdateDeviceSettings();
+  // Trust the time until response from SystemClock is received.
+  EXPECT_TRUE(device_off_hours_controller()->is_off_hours_mode());
+
+  // SystemClock is updated.
+  system_clock_client()->SetServiceIsAvailable(true);
+  system_clock_client()->SetNetworkSynchronized(false);
+  system_clock_client()->NotifyObserversSystemClockUpdated();
+  UpdateDeviceSettings();
+
+  // Response from SystemClock arrived, stop trusting the time.
+  EXPECT_FALSE(device_off_hours_controller()->is_off_hours_mode());
+}
+
 TEST_F(DeviceOffHoursControllerSimpleTest, NoNetworkSynchronization) {
+  system_clock_client()->SetServiceIsAvailable(true);
   system_clock_client()->SetNetworkSynchronized(false);
   system_clock_client()->NotifyObserversSystemClockUpdated();
   em::ChromeDeviceSettingsProto& proto(device_policy_->payload());
@@ -245,6 +279,7 @@ TEST_F(DeviceOffHoursControllerSimpleTest, NoNetworkSynchronization) {
 
 TEST_F(DeviceOffHoursControllerSimpleTest,
        IsCurrentSessionAllowedOnlyForOffHours) {
+  system_clock_client()->SetServiceIsAvailable(true);
   EXPECT_FALSE(
       device_off_hours_controller()->IsCurrentSessionAllowedOnlyForOffHours());
 
@@ -308,6 +343,7 @@ class DeviceOffHoursControllerFakeClockTest
 };
 
 TEST_F(DeviceOffHoursControllerFakeClockTest, FakeClock) {
+  system_clock_client()->SetServiceIsAvailable(true);
   EXPECT_FALSE(device_off_hours_controller()->is_off_hours_mode());
   int current_day_of_week = ExtractDayOfWeek(clock()->Now());
   em::ChromeDeviceSettingsProto& proto(device_policy_->payload());
@@ -329,6 +365,7 @@ TEST_F(DeviceOffHoursControllerFakeClockTest, FakeClock) {
 }
 
 TEST_F(DeviceOffHoursControllerFakeClockTest, CheckSendSuspendDone) {
+  system_clock_client()->SetServiceIsAvailable(true);
   int current_day_of_week = ExtractDayOfWeek(clock()->Now());
   LOG(ERROR) << "day " << current_day_of_week;
   em::ChromeDeviceSettingsProto& proto(device_policy_->payload());
@@ -362,6 +399,7 @@ class DeviceOffHoursControllerUpdateTest
 };
 
 TEST_P(DeviceOffHoursControllerUpdateTest, CheckUpdateOffHoursPolicy) {
+  system_clock_client()->SetServiceIsAvailable(true);
   em::ChromeDeviceSettingsProto& proto(device_policy_->payload());
   SetOffHoursPolicyToProto(&proto, off_hours_policy());
   AdvanceTestClock(advance_clock());

chromeos/dbus/system_clock/fake_system_clock_client.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <utility>
+
 #include "chromeos/dbus/system_clock/fake_system_clock_client.h"
 #include "base/bind.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -21,6 +23,18 @@ void FakeSystemClockClient::NotifyObserversSystemClockUpdated() {
     observer.SystemClockUpdated();
 }
 
+void FakeSystemClockClient::SetServiceIsAvailable(bool is_available) {
+  is_available_ = is_available;
+
+  if (!is_available_)
+    return;
+
+  std::vector<dbus::ObjectProxy::WaitForServiceToBeAvailableCallback> callbacks;
+  callbacks.swap(callbacks_);
+  for (auto& callback : callbacks)
+    std::move(callback).Run(true);
+}
+
 void FakeSystemClockClient::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }
@@ -46,6 +60,12 @@ void FakeSystemClockClient::GetLastSyncInfo(GetLastSyncInfoCallback callback) {
 
 void FakeSystemClockClient::WaitForServiceToBeAvailable(
     dbus::ObjectProxy::WaitForServiceToBeAvailableCallback callback) {
+  // Parent controls when callbacks are fired.
+  if (!is_available_) {
+    callbacks_.push_back(std::move(callback));
+    return;
+  }
+
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(callback), true));
 }

chromeos/dbus/system_clock/fake_system_clock_client.h

@@ -6,6 +6,7 @@
 #define CHROMEOS_DBUS_SYSTEM_CLOCK_FAKE_SYSTEM_CLOCK_CLIENT_H_
 
 #include <stdint.h>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/observer_list.h"
@@ -25,6 +26,7 @@ class COMPONENT_EXPORT(SYSTEM_CLOCK) FakeSystemClockClient
   // TestInterface
   void SetNetworkSynchronized(bool network_synchronized) override;
   void NotifyObserversSystemClockUpdated() override;
+  void SetServiceIsAvailable(bool is_available) override;
 
   // SystemClockClient overrides
   void AddObserver(Observer* observer) override;
@@ -38,8 +40,11 @@ class COMPONENT_EXPORT(SYSTEM_CLOCK) FakeSystemClockClient
   SystemClockClient::TestInterface* GetTestInterface() override;
 
  private:
+  bool is_available_ = true;
   bool network_synchronized_ = false;
 
+  std::vector<dbus::ObjectProxy::WaitForServiceToBeAvailableCallback>
+      callbacks_;
   base::ObserverList<Observer>::Unchecked observers_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSystemClockClient);

chromeos/dbus/system_clock/system_clock_client.h

@@ -46,6 +46,11 @@ class COMPONENT_EXPORT(SYSTEM_CLOCK) SystemClockClient {
 
     // Calls SystemClockUpdated for observers.
     virtual void NotifyObserversSystemClockUpdated() = 0;
+
+    // If |is_available| is false callbacks passed to
+    // WaitForServiceToBeAvailable will pile up, until |is_available| is set
+    // back to true.
+    virtual void SetServiceIsAvailable(bool is_available) = 0;
   };
 
   // Creates and initializes the global instance. |bus| must not be null.