[EasyUnlock] Update ProximityMonitor to only check for RSSI proximity.

This CL also changes UnlockManager to use the new interface for ProximityMonitor.

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

components/proximity_auth/fake_remote_device_life_cycle.cc

@@ -22,6 +22,10 @@ cryptauth::RemoteDevice FakeRemoteDeviceLifeCycle::GetRemoteDevice() const {
   return remote_device_;
 }
 
+cryptauth::Connection* FakeRemoteDeviceLifeCycle::GetConnection() const {
+  return connection_;
+}
+
 RemoteDeviceLifeCycle::State FakeRemoteDeviceLifeCycle::GetState() const {
   return state_;
 }

components/proximity_auth/fake_remote_device_life_cycle.h

@@ -7,6 +7,7 @@
 
 #include "base/macros.h"
 #include "base/observer_list.h"
+#include "components/cryptauth/fake_connection.h"
 #include "components/cryptauth/remote_device.h"
 #include "components/proximity_auth/remote_device_life_cycle.h"
 
@@ -20,6 +21,7 @@ class FakeRemoteDeviceLifeCycle : public RemoteDeviceLifeCycle {
   // RemoteDeviceLifeCycle:
   void Start() override;
   cryptauth::RemoteDevice GetRemoteDevice() const override;
+  cryptauth::Connection* GetConnection() const override;
   State GetState() const override;
   Messenger* GetMessenger() override;
   void AddObserver(Observer* observer) override;
@@ -30,6 +32,10 @@ class FakeRemoteDeviceLifeCycle : public RemoteDeviceLifeCycle {
 
   void set_messenger(Messenger* messenger) { messenger_ = messenger; }
 
+  void set_connection(cryptauth::Connection* connection) {
+    connection_ = connection;
+  }
+
   bool started() { return started_; }
 
   base::ObserverList<Observer>& observers() { return observers_; }
@@ -43,6 +49,8 @@ class FakeRemoteDeviceLifeCycle : public RemoteDeviceLifeCycle {
 
   State state_;
 
+  cryptauth::Connection* connection_;
+
   Messenger* messenger_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeRemoteDeviceLifeCycle);

components/proximity_auth/messenger.h

@@ -42,6 +42,11 @@ class Messenger {
   // Returns the SecureContext instance used by the messenger. Ownership of the
   // SecureContext is not passed.
   virtual cryptauth::SecureContext* GetSecureContext() const = 0;
+
+  // Returns the underlying raw connection. Note that you should use
+  // |GetSecureContext()| instead if you want to send and receive messages
+  // securely.
+  virtual cryptauth::Connection* GetConnection() const = 0;
 };
 
 }  // namespace proximity_auth

components/proximity_auth/messenger_impl.cc

@@ -154,6 +154,10 @@ cryptauth::SecureContext* MessengerImpl::GetSecureContext() const {
   return secure_context_.get();
 }
 
+cryptauth::Connection* MessengerImpl::GetConnection() const {
+  return connection_.get();
+}
+
 MessengerImpl::PendingMessage::PendingMessage() {}
 
 MessengerImpl::PendingMessage::PendingMessage(

components/proximity_auth/messenger_impl.h

@@ -44,6 +44,7 @@ class MessengerImpl : public Messenger, public cryptauth::ConnectionObserver {
   void RequestDecryption(const std::string& challenge) override;
   void RequestUnlock() override;
   cryptauth::SecureContext* GetSecureContext() const override;
+  cryptauth::Connection* GetConnection() const override;
 
   // Exposed for testing.
   cryptauth::Connection* connection() { return connection_.get(); }

components/proximity_auth/proximity_monitor.h

@@ -13,8 +13,6 @@ namespace proximity_auth {
 // sufficiently close to the local device to permit unlocking.
 class ProximityMonitor {
  public:
-  enum class Strategy { NONE, CHECK_RSSI, CHECK_TRANSMIT_POWER };
-
   virtual ~ProximityMonitor() {}
 
   // Activates the proximity monitor. No-op if the proximity monitor is already
@@ -25,18 +23,10 @@ class ProximityMonitor {
   // already inactive.
   virtual void Stop() = 0;
 
-  // Returns the strategy used to determine whether the remote device is in
-  // proximity.
-  virtual Strategy GetStrategy() const = 0;
-
   // Returns |true| iff the remote device is close enough to the local device,
   // given the user's current settings.
   virtual bool IsUnlockAllowed() const = 0;
 
-  // Returns |true| iff the remote device is close enough to the local device,
-  // according to its RSSI measurement.
-  virtual bool IsInRssiRange() const = 0;
-
   // Records the current proximity measurements to UMA. This should be called
   // when the user successfully authenticates using proximity auth.
   virtual void RecordProximityMetricsOnAuthSuccess() = 0;

components/proximity_auth/proximity_monitor_impl.cc

@@ -27,16 +27,15 @@ const int kPollingTimeoutMs = 250;
 
 // The RSSI threshold below which we consider the remote device to not be in
 // proximity.
-const int kRssiThreshold = -5;
+const int kRssiThreshold = -45;
 
 // The weight of the most recent RSSI sample.
 const double kRssiSampleWeight = 0.3;
 
 ProximityMonitorImpl::ProximityMonitorImpl(
-    const cryptauth::RemoteDevice& remote_device,
+    cryptauth::Connection* connection,
     std::unique_ptr<base::TickClock> clock)
-    : remote_device_(remote_device),
-      strategy_(Strategy::NONE),
+    : connection_(connection),
       remote_device_is_in_proximity_(false),
       is_active_(false),
       clock_(std::move(clock)),
@@ -50,11 +49,6 @@ ProximityMonitorImpl::ProximityMonitorImpl(
     PA_LOG(ERROR) << "[Proximity] Proximity monitoring unavailable: "
                   << "Bluetooth is unsupported on this platform.";
   }
-
-  // TODO(isherman): Test prefs to set the strategy. Need to read from "Local
-  // State" prefs on the sign-in screen, and per-user prefs on the lock screen.
-  // TODO(isherman): Unlike in the JS app, destroy and recreate the proximity
-  // monitor when the connection state changes.
 }
 
 ProximityMonitorImpl::~ProximityMonitorImpl() {
@@ -71,17 +65,8 @@ void ProximityMonitorImpl::Stop() {
   UpdatePollingState();
 }
 
-ProximityMonitor::Strategy ProximityMonitorImpl::GetStrategy() const {
-  return strategy_;
-}
-
 bool ProximityMonitorImpl::IsUnlockAllowed() const {
-  return strategy_ == Strategy::NONE || remote_device_is_in_proximity_;
-}
-
-bool ProximityMonitorImpl::IsInRssiRange() const {
-  return (strategy_ != Strategy::NONE && rssi_rolling_average_ &&
-          *rssi_rolling_average_ > kRssiThreshold);
+  return remote_device_is_in_proximity_;
 }
 
 void ProximityMonitorImpl::RecordProximityMetricsOnAuthSuccess() {
@@ -89,26 +74,12 @@ void ProximityMonitorImpl::RecordProximityMetricsOnAuthSuccess() {
                                     ? *rssi_rolling_average_
                                     : metrics::kUnknownProximityValue;
 
-  int last_transmit_power_delta =
-      last_transmit_power_reading_
-          ? (last_transmit_power_reading_->transmit_power -
-             last_transmit_power_reading_->max_transmit_power)
-          : metrics::kUnknownProximityValue;
-
-  // If no zero RSSI value has been read, then record an overflow.
-  base::TimeDelta time_since_last_zero_rssi;
-  if (last_zero_rssi_timestamp_)
-    time_since_last_zero_rssi = clock_->NowTicks() - *last_zero_rssi_timestamp_;
-  else
-    time_since_last_zero_rssi = base::TimeDelta::FromDays(100);
-
   std::string remote_device_model = metrics::kUnknownDeviceModel;
-  if (remote_device_.name != remote_device_.bluetooth_address)
-    remote_device_model = remote_device_.name;
+  cryptauth::RemoteDevice remote_device = connection_->remote_device();
+  if (!remote_device.name.empty())
+    remote_device_model = remote_device.name;
 
   metrics::RecordAuthProximityRollingRssi(round(rssi_rolling_average));
-  metrics::RecordAuthProximityTransmitPowerDelta(last_transmit_power_delta);
-  metrics::RecordAuthProximityTimeSinceLastZeroRssi(time_since_last_zero_rssi);
   metrics::RecordAuthProximityRemoteDeviceModelHash(remote_device_model);
 }
 
@@ -120,24 +91,6 @@ void ProximityMonitorImpl::RemoveObserver(ProximityMonitorObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void ProximityMonitorImpl::SetStrategy(Strategy strategy) {
-  if (strategy_ == strategy)
-    return;
-  strategy_ = strategy;
-  CheckForProximityStateChange();
-  UpdatePollingState();
-}
-
-ProximityMonitorImpl::TransmitPowerReading::TransmitPowerReading(
-    int transmit_power,
-    int max_transmit_power)
-    : transmit_power(transmit_power), max_transmit_power(max_transmit_power) {
-}
-
-bool ProximityMonitorImpl::TransmitPowerReading::IsInProximity() const {
-  return transmit_power < max_transmit_power;
-}
-
 void ProximityMonitorImpl::OnAdapterInitialized(
     scoped_refptr<device::BluetoothAdapter> adapter) {
   bluetooth_adapter_ = adapter;
@@ -170,25 +123,23 @@ void ProximityMonitorImpl::PerformScheduledUpdatePollingState() {
 }
 
 bool ProximityMonitorImpl::ShouldPoll() const {
-  // Note: We poll even if the strategy is NONE so we can record measurements.
   return is_active_ && bluetooth_adapter_;
 }
 
 void ProximityMonitorImpl::Poll() {
   DCHECK(ShouldPoll());
 
-  BluetoothDevice* device =
-      bluetooth_adapter_->GetDevice(remote_device_.bluetooth_address);
+  std::string address = connection_->GetDeviceAddress();
+  BluetoothDevice* device = bluetooth_adapter_->GetDevice(address);
 
   if (!device) {
-    PA_LOG(ERROR) << "Unknown Bluetooth device with address "
-                  << remote_device_.bluetooth_address;
+    PA_LOG(ERROR) << "Unknown Bluetooth device with address " << address;
     ClearProximityState();
     return;
   }
   if (!device->IsConnected()) {
-    PA_LOG(ERROR) << "Bluetooth device with address "
-                  << remote_device_.bluetooth_address << " is not connected.";
+    PA_LOG(ERROR) << "Bluetooth device with address " << address
+                  << " is not connected.";
     ClearProximityState();
     return;
   }
@@ -204,17 +155,12 @@ void ProximityMonitorImpl::OnConnectionInfo(
     return;
   }
 
-  if (connection_info.rssi != BluetoothDevice::kUnknownPower &&
-      connection_info.transmit_power != BluetoothDevice::kUnknownPower &&
-      connection_info.max_transmit_power != BluetoothDevice::kUnknownPower) {
+  if (connection_info.rssi != BluetoothDevice::kUnknownPower) {
     AddSample(connection_info);
   } else {
     PA_LOG(WARNING) << "[Proximity] Unkown values received from API: "
-                    << connection_info.rssi << " "
-                    << connection_info.transmit_power << " "
-                    << connection_info.max_transmit_power;
+                    << connection_info.rssi;
     rssi_rolling_average_.reset();
-    last_transmit_power_reading_.reset();
     CheckForProximityStateChange();
   }
 }
@@ -227,8 +173,6 @@ void ProximityMonitorImpl::ClearProximityState() {
 
   remote_device_is_in_proximity_ = false;
   rssi_rolling_average_.reset();
-  last_transmit_power_reading_.reset();
-  last_zero_rssi_timestamp_.reset();
 }
 
 void ProximityMonitorImpl::AddSample(
@@ -240,33 +184,16 @@ void ProximityMonitorImpl::AddSample(
     *rssi_rolling_average_ =
         weight * connection_info.rssi + (1 - weight) * (*rssi_rolling_average_);
   }
-  last_transmit_power_reading_.reset(new TransmitPowerReading(
-      connection_info.transmit_power, connection_info.max_transmit_power));
-
-  // It's rare but possible for the RSSI to be positive briefly.
-  if (connection_info.rssi >= 0)
-    last_zero_rssi_timestamp_.reset(new base::TimeTicks(clock_->NowTicks()));
 
   CheckForProximityStateChange();
 }
 
 void ProximityMonitorImpl::CheckForProximityStateChange() {
-  if (strategy_ == Strategy::NONE)
-    return;
-
-  bool is_now_in_proximity = false;
-  switch (strategy_) {
-    case Strategy::NONE:
-      return;
-
-    case Strategy::CHECK_RSSI:
-      is_now_in_proximity = IsInRssiRange();
-      break;
+  bool is_now_in_proximity =
+      rssi_rolling_average_ && *rssi_rolling_average_ > kRssiThreshold;
 
-    case Strategy::CHECK_TRANSMIT_POWER:
-      is_now_in_proximity = (last_transmit_power_reading_ &&
-                             last_transmit_power_reading_->IsInProximity());
-      break;
+  if (rssi_rolling_average_) {
+    LOG(WARNING) << "RSSI: " << *rssi_rolling_average_;
   }
 
   if (remote_device_is_in_proximity_ != is_now_in_proximity) {

components/proximity_auth/proximity_monitor_impl.h

@@ -11,13 +11,13 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "components/cryptauth/connection.h"
 #include "components/cryptauth/remote_device.h"
 #include "components/proximity_auth/proximity_monitor.h"
 #include "device/bluetooth/bluetooth_device.h"
 
 namespace base {
 class TickClock;
-class TimeTicks;
 }
 
 namespace device {
@@ -31,41 +31,20 @@ class ProximityMonitorObserver;
 // The concrete implemenation of the proximity monitor interface.
 class ProximityMonitorImpl : public ProximityMonitor {
  public:
-  // The |observer| is not owned, and must outlive |this| instance.
-  ProximityMonitorImpl(const cryptauth::RemoteDevice& remote_device,
+  // The |connection| is not owned, and must outlive |this| instance.
+  ProximityMonitorImpl(cryptauth::Connection* connection,
                        std::unique_ptr<base::TickClock> clock);
   ~ProximityMonitorImpl() override;
 
   // ProximityMonitor:
   void Start() override;
   void Stop() override;
-  Strategy GetStrategy() const override;
   bool IsUnlockAllowed() const override;
-  bool IsInRssiRange() const override;
   void RecordProximityMetricsOnAuthSuccess() override;
   void AddObserver(ProximityMonitorObserver* observer) override;
   void RemoveObserver(ProximityMonitorObserver* observer) override;
 
- protected:
-  // Sets the proximity detection strategy. Exposed for testing.
-  // TODO(isherman): Stop exposing this for testing once prefs are properly
-  // hooked up.
-  virtual void SetStrategy(Strategy strategy);
-
  private:
-  struct TransmitPowerReading {
-    TransmitPowerReading(int transmit_power, int max_transmit_power);
-
-    // Returns true if |this| transmit power reading indicates proximity.
-    bool IsInProximity() const;
-
-    // The current transmit power.
-    int transmit_power;
-
-    // The maximum possible transmit power.
-    int max_transmit_power;
-  };
-
   // Callback for asynchronous initialization of the Bluetooth adpater.
   void OnAdapterInitialized(scoped_refptr<device::BluetoothAdapter> adapter);
 
@@ -95,7 +74,7 @@ class ProximityMonitorImpl : public ProximityMonitor {
   void ClearProximityState();
 
   // Updates the proximity state with a new |connection_info| sample of the
-  // current RSSI and Tx power, and the device's maximum Tx power.
+  // current RSSI.
   void AddSample(
       const device::BluetoothDevice::ConnectionInfo& connection_info);
 
@@ -103,8 +82,9 @@ class ProximityMonitorImpl : public ProximityMonitor {
   // samples. Notifies |observers_| on a change.
   void CheckForProximityStateChange();
 
-  // The remote device being monitored.
-  const cryptauth::RemoteDevice remote_device_;
+  // The current connection being monitored. Not owned and must outlive this
+  // instance.
+  cryptauth::Connection* connection_;
 
   // The observers attached to the ProximityMonitor.
   base::ObserverList<ProximityMonitorObserver> observers_;
@@ -112,9 +92,6 @@ class ProximityMonitorImpl : public ProximityMonitor {
   // The Bluetooth adapter that will be polled for connection info.
   scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
 
-  // The strategy used to determine whether the remote device is in proximity.
-  Strategy strategy_;
-
   // Whether the remote device is currently in close proximity to the local
   // device.
   bool remote_device_is_in_proximity_;
@@ -129,18 +106,6 @@ class ProximityMonitorImpl : public ProximityMonitor {
   // measurement.
   std::unique_ptr<double> rssi_rolling_average_;
 
-  // The last TX power reading. Null if the monitor is inactive, has not
-  // recently observed a TX power reading, or the most recent connection info
-  // included an invalid measurement.
-  std::unique_ptr<TransmitPowerReading> last_transmit_power_reading_;
-
-  // The timestamp of the last zero RSSI reading. An RSSI value of 0 is special
-  // because both devices adjust their transmit powers such that the RSSI is in
-  // this golden range, if possible. Null if the monitor is inactive, has not
-  // recently observed an RSSI reading, or the most recent connection info
-  // included an invalid measurement.
-  std::unique_ptr<base::TimeTicks> last_zero_rssi_timestamp_;
-
   // Used to access non-decreasing time measurements.
   std::unique_ptr<base::TickClock> clock_;
 

components/proximity_auth/remote_device_life_cycle.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_PROXIMITY_AUTH_REMOTE_DEVICE_LIFE_CYCLE_H
 
 #include "base/macros.h"
+#include "components/cryptauth/connection.h"
 #include "components/cryptauth/remote_device.h"
 
 namespace proximity_auth {
@@ -57,6 +58,9 @@ class RemoteDeviceLifeCycle {
   // Returns the RemoteDevice instance that this life cycle manages.
   virtual cryptauth::RemoteDevice GetRemoteDevice() const = 0;
 
+  // Returns the current Connection, or null if the device is not yet connected.
+  virtual cryptauth::Connection* GetConnection() const = 0;
+
   // Returns the current state of in the life cycle.
   virtual State GetState() const = 0;
 

components/proximity_auth/remote_device_life_cycle_impl.cc

@@ -62,6 +62,14 @@ cryptauth::RemoteDevice RemoteDeviceLifeCycleImpl::GetRemoteDevice() const {
   return remote_device_;
 }
 
+cryptauth::Connection* RemoteDeviceLifeCycleImpl::GetConnection() const {
+  if (connection_)
+    return connection_.get();
+  if (messenger_)
+    return messenger_->GetConnection();
+  return nullptr;
+}
+
 RemoteDeviceLifeCycle::State RemoteDeviceLifeCycleImpl::GetState() const {
   return state_;
 }

components/proximity_auth/remote_device_life_cycle_impl.h

@@ -41,6 +41,7 @@ class RemoteDeviceLifeCycleImpl : public RemoteDeviceLifeCycle,
   // RemoteDeviceLifeCycle:
   void Start() override;
   cryptauth::RemoteDevice GetRemoteDevice() const override;
+  cryptauth::Connection* GetConnection() const override;
   RemoteDeviceLifeCycle::State GetState() const override;
   Messenger* GetMessenger() override;
   void AddObserver(Observer* observer) override;

components/proximity_auth/unlock_manager_impl.cc

@@ -141,7 +141,6 @@ void UnlockManagerImpl::SetRemoteDeviceLifeCycle(
 
   life_cycle_ = life_cycle;
   if (life_cycle_) {
-    proximity_monitor_ = CreateProximityMonitor(life_cycle->GetRemoteDevice());
     SetWakingUpState(true);
   } else {
     proximity_monitor_.reset();
@@ -156,8 +155,12 @@ void UnlockManagerImpl::OnLifeCycleStateChanged() {
                << static_cast<int>(state);
 
   remote_screenlock_state_.reset();
-  if (state == RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED)
+  if (state == RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED) {
+    DCHECK(life_cycle_->GetConnection());
+    DCHECK(GetMessenger());
+    proximity_monitor_ = CreateProximityMonitor(life_cycle_->GetConnection());
     GetMessenger()->AddObserver(this);
+  }
 
   if (state == RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED)
     SetWakingUpState(false);
@@ -320,9 +323,9 @@ void UnlockManagerImpl::OnAuthAttempted(
 }
 
 std::unique_ptr<ProximityMonitor> UnlockManagerImpl::CreateProximityMonitor(
-    const cryptauth::RemoteDevice& remote_device) {
+    cryptauth::Connection* connection) {
   return base::MakeUnique<ProximityMonitorImpl>(
-      remote_device, base::WrapUnique(new base::DefaultTickClock()));
+      connection, base::WrapUnique(new base::DefaultTickClock()));
 }
 
 void UnlockManagerImpl::SendSignInChallenge() {
@@ -356,7 +359,10 @@ ScreenlockState UnlockManagerImpl::GetScreenlockState() {
       RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED)
     return ScreenlockState::PHONE_NOT_AUTHENTICATED;
 
-  if (is_waking_up_)
+  if (is_waking_up_ ||
+      life_cycle_->GetState() == RemoteDeviceLifeCycle::State::AUTHENTICATING ||
+      life_cycle_->GetState() ==
+          RemoteDeviceLifeCycle::State::FINDING_CONNECTION)
     return ScreenlockState::BLUETOOTH_CONNECTING;
 
   if (!bluetooth_adapter_ || !bluetooth_adapter_->IsPowered())
@@ -370,8 +376,7 @@ ScreenlockState UnlockManagerImpl::GetScreenlockState() {
   // If the RSSI is too low, then the remote device is nowhere near the local
   // device. This message should take priority over messages about screen lock
   // states.
-  if (!proximity_monitor_->IsUnlockAllowed() &&
-      !proximity_monitor_->IsInRssiRange())
+  if (!proximity_monitor_->IsUnlockAllowed())
     return ScreenlockState::RSSI_TOO_LOW;
 
   if (remote_screenlock_state_) {
@@ -380,11 +385,6 @@ ScreenlockState UnlockManagerImpl::GetScreenlockState() {
         return ScreenlockState::PHONE_NOT_LOCKABLE;
 
       case RemoteScreenlockState::LOCKED:
-        if (proximity_monitor_->GetStrategy() ==
-                ProximityMonitor::Strategy::CHECK_TRANSMIT_POWER &&
-            !proximity_monitor_->IsUnlockAllowed()) {
-          return ScreenlockState::PHONE_LOCKED_AND_TX_POWER_TOO_HIGH;
-        }
         return ScreenlockState::PHONE_LOCKED;
 
       case RemoteScreenlockState::UNKNOWN:
@@ -396,18 +396,6 @@ ScreenlockState UnlockManagerImpl::GetScreenlockState() {
     }
   }
 
-  if (!proximity_monitor_->IsUnlockAllowed()) {
-    ProximityMonitor::Strategy strategy = proximity_monitor_->GetStrategy();
-    if (strategy != ProximityMonitor::Strategy::CHECK_TRANSMIT_POWER) {
-      // CHECK_RSSI should have been handled above, and no other states should
-      // prevent unlocking.
-      PA_LOG(ERROR) << "[Unlock] Invalid ProximityMonitor strategy: "
-                    << static_cast<int>(strategy);
-      return ScreenlockState::NO_PHONE;
-    }
-    return ScreenlockState::TX_POWER_TOO_HIGH;
-  }
-
   return ScreenlockState::NO_PHONE;
 }
 

components/proximity_auth/unlock_manager_impl.h

@@ -52,10 +52,10 @@ class UnlockManagerImpl : public UnlockManager,
       ScreenlockBridge::LockHandler::AuthType auth_type) override;
 
  protected:
-  // Creates a ProximityMonitor instance for the given |remote_device|.
+  // Creates a ProximityMonitor instance for the given |connection|.
   // Exposed for testing.
   virtual std::unique_ptr<ProximityMonitor> CreateProximityMonitor(
-      const cryptauth::RemoteDevice& remote_device);
+      cryptauth::Connection* connection);
 
  private:
   // The possible lock screen states for the remote device.