Use derived key from SyncService

- Refactored VAPID key manager to separate get key and refresh key
- Derive from sync secret if kSharingDeriveVapidKey is enabled
- Observe for VAPID key source changes, refresh cached key and
  re-register if cached VAPID key has changed

Bug: 1010968
Change-Id: I178a6cd931ad41ab0184dd8a511e8ac0569e2269
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1848171
Commit-Queue: Alex Chau <alexchau@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706399}

chrome/browser/sharing/sharing_device_registration_unittest.cc

@@ -4,15 +4,18 @@
 
 #include "chrome/browser/sharing/sharing_device_registration.h"
 
+#include <stdint.h>
 #include <map>
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "build/build_config.h"
+#include "chrome/browser/sharing/features.h"
 #include "chrome/browser/sharing/shared_clipboard/feature_flags.h"
 #include "chrome/browser/sharing/sharing_constants.h"
 #include "chrome/browser/sharing/sharing_device_registration_result.h"
@@ -22,11 +25,13 @@
 #include "components/gcm_driver/instance_id/instance_id_driver.h"
 #include "components/prefs/pref_registry.h"
 #include "components/prefs/pref_service_factory.h"
+#include "components/sync/driver/test_sync_service.h"
 #include "components/sync_device_info/device_info.h"
 #include "components/sync_device_info/fake_device_info_sync_service.h"
 #include "components/sync_preferences/pref_service_mock_factory.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/browser_task_environment.h"
+#include "crypto/ec_private_key.h"
 #include "google_apis/gcm/engine/account_mapping.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -118,7 +123,7 @@ class SharingDeviceRegistrationTest : public testing::Test {
  public:
   SharingDeviceRegistrationTest()
       : sync_prefs_(&prefs_, &fake_device_info_sync_service_),
-        vapid_key_manager_(&sync_prefs_),
+        vapid_key_manager_(&sync_prefs_, &test_sync_service_),
         sharing_device_registration_(pref_service_.get(),
                                      &sync_prefs_,
                                      &mock_instance_id_driver_,
@@ -213,6 +218,7 @@ class SharingDeviceRegistrationTest : public testing::Test {
   std::unique_ptr<PrefService> pref_service_ =
       CreatePrefServiceAndRegisterPrefs();
   SharingSyncPreference sync_prefs_;
+  syncer::TestSyncService test_sync_service_;
   VapidKeyManager vapid_key_manager_;
   SharingDeviceRegistration sharing_device_registration_;
 
@@ -258,9 +264,14 @@ TEST_F(SharingDeviceRegistrationTest, RegisterDeviceTest_Success) {
   EXPECT_EQ(expected_sharing_info, synced_sharing_info_);
   EXPECT_TRUE(fcm_registration_);
 
-  // Remove VAPID key to force a re-register, which will return a different FCM
+  // Change VAPID key to force a re-register, which will return a different FCM
   // token.
-  prefs_.RemoveUserPref("sharing.vapid_key");
+  auto vapid_key = crypto::ECPrivateKey::Create();
+  ASSERT_TRUE(vapid_key);
+  std::vector<uint8_t> vapid_key_info;
+  ASSERT_TRUE(vapid_key->ExportPrivateKey(&vapid_key_info));
+  sync_prefs_.SetVapidKey(vapid_key_info);
+  vapid_key_manager_.RefreshCachedKey();
   SetInstanceIDFCMToken(kFCMToken2);
 
   RegisterDeviceSync();

chrome/browser/sharing/sharing_fcm_sender.cc

@@ -73,6 +73,14 @@ void SharingFCMSender::DoSendMessageToDevice(
     return;
   }
 
+  auto* vapid_key = vapid_key_manager_->GetOrCreateKey();
+  if (!vapid_key) {
+    LOG(ERROR) << "Unable to retrieve VAPID key";
+    std::move(callback).Run(SharingSendMessageResult::kInternalError,
+                            base::nullopt);
+    return;
+  }
+
   const syncer::DeviceInfo* local_device_info =
       device_info_provider_->GetLocalDeviceInfo();
   message.set_sender_guid(local_device_info->guid());
@@ -92,8 +100,8 @@ void SharingFCMSender::DoSendMessageToDevice(
 
   gcm_driver_->SendWebPushMessage(
       kSharingFCMAppID, fcm_registration->authorized_entity, target.p256dh,
-      target.auth_secret, target.fcm_token,
-      vapid_key_manager_->GetOrCreateKey(), std::move(web_push_message),
+      target.auth_secret, target.fcm_token, vapid_key,
+      std::move(web_push_message),
       base::BindOnce(&SharingFCMSender::OnMessageSent,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }

chrome/browser/sharing/sharing_fcm_sender_unittest.cc

@@ -76,7 +76,9 @@ class FakeGCMDriver : public gcm::FakeGCMDriver {
 
 class MockVapidKeyManager : public VapidKeyManager {
  public:
-  MockVapidKeyManager() : VapidKeyManager(nullptr) {}
+  MockVapidKeyManager()
+      : VapidKeyManager(/*sharing_sync_preference=*/nullptr,
+                        /*sync_service=*/nullptr) {}
   ~MockVapidKeyManager() {}
 
   MOCK_METHOD0(GetOrCreateKey, crypto::ECPrivateKey*());
@@ -93,30 +95,92 @@ class SharingFCMSenderTest : public testing::Test {
   }
 
  protected:
-  SharingFCMSenderTest() {
-    // TODO: Used fake GCMDriver
-    sync_prefs_ = std::make_unique<SharingSyncPreference>(
-        &prefs_, &fake_device_info_sync_service);
-    sharing_fcm_sender_ = std::make_unique<SharingFCMSender>(
-        &fake_gcm_driver_,
-        fake_device_info_sync_service.GetLocalDeviceInfoProvider(),
-        sync_prefs_.get(), &vapid_key_manager_);
+  SharingFCMSenderTest()
+      : sync_prefs_(&prefs_, &fake_device_info_sync_service),
+        sharing_fcm_sender_(
+            &fake_gcm_driver_,
+            fake_device_info_sync_service.GetLocalDeviceInfoProvider(),
+            &sync_prefs_,
+            &vapid_key_manager_) {
     SharingSyncPreference::RegisterProfilePrefs(prefs_.registry());
   }
 
   syncer::FakeDeviceInfoSyncService fake_device_info_sync_service;
   FakeGCMDriver fake_gcm_driver_;
 
-  std::unique_ptr<SharingSyncPreference> sync_prefs_;
-  std::unique_ptr<SharingFCMSender> sharing_fcm_sender_;
+  SharingSyncPreference sync_prefs_;
   testing::NiceMock<MockVapidKeyManager> vapid_key_manager_;
+  SharingFCMSender sharing_fcm_sender_;
 
  private:
   sync_preferences::TestingPrefServiceSyncable prefs_;
-};
+};  // namespace
 
 }  // namespace
 
+TEST_F(SharingFCMSenderTest, NoFcmRegistration) {
+  sync_prefs_.ClearFCMRegistration();
+  syncer::DeviceInfo* local_device_info =
+      fake_device_info_sync_service.GetLocalDeviceInfoProvider()
+          ->GetMutableDeviceInfo();
+  local_device_info->set_sharing_info(syncer::DeviceInfo::SharingInfo(
+      kSenderFcmToken, kSenderP256dh, kSenderAuthSecret,
+      std::set<sync_pb::SharingSpecificFields::EnabledFeatures>()));
+  fake_device_info_sync_service.GetLocalDeviceInfoProvider()->SetReady(true);
+
+  std::unique_ptr<crypto::ECPrivateKey> vapid_key =
+      crypto::ECPrivateKey::Create();
+  ON_CALL(vapid_key_manager_, GetOrCreateKey())
+      .WillByDefault(testing::Return(vapid_key.get()));
+
+  syncer::DeviceInfo::SharingInfo target(
+      kFcmToken, kP256dh, kAuthSecret,
+      std::set<sync_pb::SharingSpecificFields::EnabledFeatures>());
+
+  SharingSendMessageResult result;
+  base::Optional<std::string> message_id;
+  chrome_browser_sharing::SharingMessage sharing_message;
+  sharing_message.mutable_ping_message();
+  sharing_fcm_sender_.SendMessageToDevice(
+      std::move(target), base::TimeDelta::FromSeconds(kTtlSeconds),
+      std::move(sharing_message),
+      base::BindOnce(&SharingFCMSenderTest::OnMessageSent,
+                     base::Unretained(this), &result, &message_id));
+
+  EXPECT_EQ(SharingSendMessageResult::kInternalError, result);
+}
+
+TEST_F(SharingFCMSenderTest, NoVapidKey) {
+  sync_prefs_.SetFCMRegistration(SharingSyncPreference::FCMRegistration(
+      kAuthorizedEntity, base::Time::Now()));
+  syncer::DeviceInfo* local_device_info =
+      fake_device_info_sync_service.GetLocalDeviceInfoProvider()
+          ->GetMutableDeviceInfo();
+  local_device_info->set_sharing_info(syncer::DeviceInfo::SharingInfo(
+      kSenderFcmToken, kSenderP256dh, kSenderAuthSecret,
+      std::set<sync_pb::SharingSpecificFields::EnabledFeatures>()));
+  fake_device_info_sync_service.GetLocalDeviceInfoProvider()->SetReady(true);
+
+  ON_CALL(vapid_key_manager_, GetOrCreateKey())
+      .WillByDefault(testing::Return(nullptr));
+
+  syncer::DeviceInfo::SharingInfo target(
+      kFcmToken, kP256dh, kAuthSecret,
+      std::set<sync_pb::SharingSpecificFields::EnabledFeatures>());
+
+  SharingSendMessageResult result;
+  base::Optional<std::string> message_id;
+  chrome_browser_sharing::SharingMessage sharing_message;
+  sharing_message.mutable_ping_message();
+  sharing_fcm_sender_.SendMessageToDevice(
+      std::move(target), base::TimeDelta::FromSeconds(kTtlSeconds),
+      std::move(sharing_message),
+      base::BindOnce(&SharingFCMSenderTest::OnMessageSent,
+                     base::Unretained(this), &result, &message_id));
+
+  EXPECT_EQ(SharingSendMessageResult::kInternalError, result);
+}
+
 struct SharingFCMSenderResultTestData {
   const gcm::SendWebPushMessageResult web_push_result;
   const SharingSendMessageResult expected_result;
@@ -158,7 +222,7 @@ class SharingFCMSenderResultTest
       public testing::WithParamInterface<SharingFCMSenderResultTestData> {};
 
 TEST_P(SharingFCMSenderResultTest, ResultTest) {
-  sync_prefs_->SetFCMRegistration(SharingSyncPreference::FCMRegistration(
+  sync_prefs_.SetFCMRegistration(SharingSyncPreference::FCMRegistration(
       kAuthorizedEntity, base::Time::Now()));
   syncer::DeviceInfo* local_device_info =
       fake_device_info_sync_service.GetLocalDeviceInfoProvider()
@@ -183,7 +247,7 @@ TEST_P(SharingFCMSenderResultTest, ResultTest) {
   base::Optional<std::string> message_id;
   chrome_browser_sharing::SharingMessage sharing_message;
   sharing_message.mutable_ping_message();
-  sharing_fcm_sender_->SendMessageToDevice(
+  sharing_fcm_sender_.SendMessageToDevice(
       std::move(target), base::TimeDelta::FromSeconds(kTtlSeconds),
       std::move(sharing_message),
       base::BindOnce(&SharingFCMSenderTest::OnMessageSent,

chrome/browser/sharing/sharing_service.cc

@@ -442,6 +442,20 @@ void SharingService::OnStateChanged(syncer::SyncService* sync) {
   }
 }
 
+void SharingService::OnSyncCycleCompleted(syncer::SyncService* sync) {
+  if (!base::FeatureList::IsEnabled(kSharingDeriveVapidKey) ||
+      state_ != State::ACTIVE) {
+    return;
+  }
+
+  RefreshVapidKey();
+}
+
+void SharingService::RefreshVapidKey() {
+  if (vapid_key_manager_->RefreshCachedKey())
+    RegisterDevice();
+}
+
 void SharingService::RegisterDevice() {
   sharing_device_registration_->RegisterDevice(base::BindOnce(
       &SharingService::OnDeviceRegistered, weak_ptr_factory_.GetWeakPtr()));
@@ -471,10 +485,16 @@ void SharingService::OnDeviceRegistered(
           state_ = State::ACTIVE;
           fcm_handler_->StartListening();
 
-          // Listen for further VAPID key changes for re-registration.
-          // state_ is kept as State::ACTIVE during re-registration.
-          sync_prefs_->SetVapidKeyChangeObserver(base::BindRepeating(
-              &SharingService::RegisterDevice, weak_ptr_factory_.GetWeakPtr()));
+          if (base::FeatureList::IsEnabled(kSharingDeriveVapidKey)) {
+            // Refresh VAPID key in case it's changed during registration.
+            RefreshVapidKey();
+          } else {
+            // Listen for further VAPID key changes for re-registration.
+            // state_ is kept as State::ACTIVE during re-registration.
+            sync_prefs_->SetVapidKeyChangeObserver(
+                base::BindRepeating(&SharingService::RefreshVapidKey,
+                                    weak_ptr_factory_.GetWeakPtr()));
+          }
         } else if (IsSyncDisabled()) {
           // In case sync is disabled during registration, unregister it.
           state_ = State::UNREGISTERING;

chrome/browser/sharing/sharing_service.h

@@ -130,6 +130,7 @@ class SharingService : public KeyedService,
   // Overrides for syncer::SyncServiceObserver.
   void OnSyncShutdown(syncer::SyncService* sync) override;
   void OnStateChanged(syncer::SyncService* sync) override;
+  void OnSyncCycleCompleted(syncer::SyncService* sync) override;
 
   // AckMessageHandler::AckMessageObserver override.
   void OnAckReceived(const std::string& message_id) override;
@@ -137,6 +138,8 @@ class SharingService : public KeyedService,
   // syncer::DeviceInfoTracker::Observer.
   void OnDeviceInfoChange() override;
 
+  void RefreshVapidKey();
+
   void RegisterDevice();
   void UnregisterDevice();
 

chrome/browser/sharing/sharing_service_factory.cc

@@ -87,7 +87,7 @@ KeyedService* SharingServiceFactory::BuildServiceInstanceFor(
       std::make_unique<SharingSyncPreference>(profile->GetPrefs(),
                                               device_info_sync_service);
   std::unique_ptr<VapidKeyManager> vapid_key_manager =
-      std::make_unique<VapidKeyManager>(sync_prefs.get());
+      std::make_unique<VapidKeyManager>(sync_prefs.get(), sync_service);
   std::unique_ptr<SharingDeviceRegistration> sharing_device_registration =
       std::make_unique<SharingDeviceRegistration>(
           profile->GetPrefs(), sync_prefs.get(), instance_id_service->driver(),

chrome/browser/sharing/sharing_service_unittest.cc

@@ -30,6 +30,7 @@
 #include "components/sync_device_info/local_device_info_provider.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/browser_task_environment.h"
+#include "crypto/ec_private_key.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -134,12 +135,15 @@ class FakeSharingDeviceRegistration : public SharingDeviceRegistration {
       : SharingDeviceRegistration(pref_service,
                                   prefs,
                                   instance_id_driver,
-                                  vapid_key_manager) {}
+                                  vapid_key_manager),
+        vapid_key_manager_(vapid_key_manager) {}
   ~FakeSharingDeviceRegistration() override = default;
 
   void RegisterDevice(
       SharingDeviceRegistration::RegistrationCallback callback) override {
     registration_attempts_++;
+    // Simulate SharingDeviceRegistration calling GetOrCreateKey.
+    vapid_key_manager_->GetOrCreateKey();
     std::move(callback).Run(result_);
   }
 
@@ -163,6 +167,7 @@ class FakeSharingDeviceRegistration : public SharingDeviceRegistration {
   }
 
  private:
+  VapidKeyManager* vapid_key_manager_;
   SharingDeviceRegistrationResult result_ =
       SharingDeviceRegistrationResult::kSuccess;
   int registration_attempts_ = 0;
@@ -175,11 +180,11 @@ class SharingServiceTest : public testing::Test {
   SharingServiceTest() {
     sync_prefs_ =
         new SharingSyncPreference(&prefs_, &fake_device_info_sync_service);
+    vapid_key_manager_ = new VapidKeyManager(sync_prefs_, &test_sync_service_);
     sharing_device_registration_ = new FakeSharingDeviceRegistration(
         /* pref_service= */ nullptr, sync_prefs_, &mock_instance_id_driver_,
         vapid_key_manager_,
         fake_device_info_sync_service.GetLocalDeviceInfoProvider());
-    vapid_key_manager_ = new VapidKeyManager(sync_prefs_);
     fcm_sender_ = new SharingFCMSender(
         &fake_gcm_driver_,
         fake_device_info_sync_service.GetLocalDeviceInfoProvider(), sync_prefs_,
@@ -515,11 +520,14 @@ TEST_F(SharingServiceTest, DeviceRegistration) {
   EXPECT_EQ(1, sharing_device_registration_->registration_attempts());
   EXPECT_EQ(SharingService::State::ACTIVE, GetSharingService()->GetState());
 
-  // Registration will be attempeted as VAPID key is cleared.
+  auto vapid_key = crypto::ECPrivateKey::Create();
+  ASSERT_TRUE(vapid_key);
+  std::vector<uint8_t> vapid_key_info;
+  ASSERT_TRUE(vapid_key->ExportPrivateKey(&vapid_key_info));
+
+  // Registration will be attempeted as VAPID key has changed.
   EXPECT_CALL(*fcm_handler_, StartListening()).Times(0);
-  prefs_.SetUserPref("sharing.vapid_key",
-                     base::Value::ToUniquePtrValue(
-                         base::Value(base::Value::Type::DICTIONARY)));
+  sync_prefs_->SetVapidKey(vapid_key_info);
   EXPECT_EQ(2, sharing_device_registration_->registration_attempts());
   EXPECT_EQ(SharingService::State::ACTIVE, GetSharingService()->GetState());
 }
@@ -549,6 +557,8 @@ TEST_F(SharingServiceTest, DeviceRegistrationTransportMode) {
   test_sync_service_.SetTransportState(
       syncer::SyncService::TransportState::ACTIVE);
   test_sync_service_.SetActiveDataTypes(syncer::DEVICE_INFO);
+  test_sync_service_.SetExperimentalAuthenticationKey(
+      crypto::ECPrivateKey::Create());
 
   EXPECT_EQ(SharingService::State::DISABLED, GetSharingService()->GetState());
 
@@ -559,6 +569,14 @@ TEST_F(SharingServiceTest, DeviceRegistrationTransportMode) {
   test_sync_service_.FireStateChanged();
   EXPECT_EQ(1, sharing_device_registration_->registration_attempts());
   EXPECT_EQ(SharingService::State::ACTIVE, GetSharingService()->GetState());
+
+  // Registration will be attempeted as sync auth id has changed.
+  EXPECT_CALL(*fcm_handler_, StartListening()).Times(0);
+  test_sync_service_.SetExperimentalAuthenticationKey(
+      crypto::ECPrivateKey::Create());
+  test_sync_service_.FireSyncCycleCompleted();
+  EXPECT_EQ(2, sharing_device_registration_->registration_attempts());
+  EXPECT_EQ(SharingService::State::ACTIVE, GetSharingService()->GetState());
 }
 
 TEST_F(SharingServiceTest, DeviceRegistrationTransientError) {

chrome/browser/sharing/vapid_key_manager.cc

@@ -4,41 +4,79 @@
 
 #include "chrome/browser/sharing/vapid_key_manager.h"
 
+#include "base/feature_list.h"
+#include "chrome/browser/sharing/features.h"
 #include "chrome/browser/sharing/sharing_metrics.h"
 #include "chrome/browser/sharing/sharing_sync_preference.h"
+#include "components/sync/driver/sync_service.h"
 #include "crypto/ec_private_key.h"
 
-VapidKeyManager::VapidKeyManager(SharingSyncPreference* sharing_sync_preference)
-    : sharing_sync_preference_(sharing_sync_preference) {}
+VapidKeyManager::VapidKeyManager(SharingSyncPreference* sharing_sync_preference,
+                                 syncer::SyncService* sync_service)
+    : sharing_sync_preference_(sharing_sync_preference),
+      sync_service_(sync_service) {}
 
 VapidKeyManager::~VapidKeyManager() = default;
 
 crypto::ECPrivateKey* VapidKeyManager::GetOrCreateKey() {
-  base::Optional<std::vector<uint8_t>> stored_key =
-      sharing_sync_preference_->GetVapidKey();
+  if (!vapid_key_)
+    RefreshCachedKey();
 
-  if (stored_key) {
-    vapid_key_ = crypto::ECPrivateKey::CreateFromPrivateKeyInfo(*stored_key);
-    return vapid_key_.get();
-  }
+  return vapid_key_.get();
+}
 
-  vapid_key_ = crypto::ECPrivateKey::Create();
-  if (!vapid_key_) {
-    LogSharingVapidKeyCreationResult(
-        SharingVapidKeyCreationResult::kGenerateECKeyFailed);
-    return nullptr;
+bool VapidKeyManager::RefreshCachedKey() {
+  if (base::FeatureList::IsEnabled(kSharingDeriveVapidKey)) {
+    auto derived_key = sync_service_->GetExperimentalAuthenticationKey();
+    if (!derived_key)
+      return InitWithPreference();
+
+    return UpdateCachedKey(std::move(derived_key));
+  } else {
+    if (InitWithPreference())
+      return true;
+
+    if (vapid_key_)
+      return false;
+
+    auto generated_key = crypto::ECPrivateKey::Create();
+    if (!generated_key) {
+      LogSharingVapidKeyCreationResult(
+          SharingVapidKeyCreationResult::kGenerateECKeyFailed);
+      return false;
+    }
+
+    return UpdateCachedKey(std::move(generated_key));
   }
+}
 
-  std::vector<uint8_t> key;
-  if (!vapid_key_->ExportPrivateKey(&key)) {
-    LOG(ERROR) << "Could not export vapid key";
-    vapid_key_.reset();
+bool VapidKeyManager::UpdateCachedKey(
+    std::unique_ptr<crypto::ECPrivateKey> new_key) {
+  std::vector<uint8_t> new_key_info;
+  if (!new_key->ExportPrivateKey(&new_key_info)) {
     LogSharingVapidKeyCreationResult(
         SharingVapidKeyCreationResult::kExportPrivateKeyFailed);
-    return nullptr;
+    return false;
   }
 
-  sharing_sync_preference_->SetVapidKey(key);
+  if (vapid_key_info_ == new_key_info)
+    return false;
+
+  vapid_key_ = std::move(new_key);
+  vapid_key_info_ = std::move(new_key_info);
+  sharing_sync_preference_->SetVapidKey(vapid_key_info_);
   LogSharingVapidKeyCreationResult(SharingVapidKeyCreationResult::kSuccess);
-  return vapid_key_.get();
+  return true;
+}
+
+bool VapidKeyManager::InitWithPreference() {
+  base::Optional<std::vector<uint8_t>> preference_key_info =
+      sharing_sync_preference_->GetVapidKey();
+  if (!preference_key_info || vapid_key_info_ == *preference_key_info)
+    return false;
+
+  vapid_key_ =
+      crypto::ECPrivateKey::CreateFromPrivateKeyInfo(*preference_key_info);
+  vapid_key_info_ = std::move(*preference_key_info);
+  return true;
 }

chrome/browser/sharing/vapid_key_manager.h

@@ -5,7 +5,9 @@
 #ifndef CHROME_BROWSER_SHARING_VAPID_KEY_MANAGER_H_
 #define CHROME_BROWSER_SHARING_VAPID_KEY_MANAGER_H_
 
+#include <stdint.h>
 #include <memory>
+#include <vector>
 
 #include "base/macros.h"
 
@@ -13,7 +15,12 @@ namespace crypto {
 class ECPrivateKey;
 }  // namespace crypto
 
+namespace syncer {
+class SyncService;
+}  // namespace syncer
+
 class SharingSyncPreference;
+enum class SharingVapidKeyCreationResult;
 
 // Responsible for creating, storing and managing VAPID key. VAPID key is
 // shared across all devices for a single user and is used for signing VAPID
@@ -22,21 +29,43 @@ class SharingSyncPreference;
 // https://developers.google.com/web/fundamentals/push-notifications/web-push-protocol
 class VapidKeyManager {
  public:
-  explicit VapidKeyManager(SharingSyncPreference* sharing_sync_preference);
+  explicit VapidKeyManager(SharingSyncPreference* sharing_sync_preference,
+                           syncer::SyncService* sync_service);
   virtual ~VapidKeyManager();
 
-  // Returns the shared VAPID key stored in SharingSyncPreference. If no key is
-  // found in preferences, it generates a new key and stores in
-  // SharingSyncPreference before returning this new key. Conflicts between
-  // different devices generating the shared VAPID key is resolved based on
-  // creation time.
+  // Returns the cached key. If absent, first attempts to refresh the cached
+  // key. May return nullptr if cached key is absent after refresh.
   virtual crypto::ECPrivateKey* GetOrCreateKey();
 
+  // Attempts to refresh cached key from various source and returns if cached
+  // key has changed.
+  //
+  // If kSharingDeriveVapidKey is enabled:
+  // 1. Attempts to derive a key from sync secret. If successful, cache it and
+  // store in sync preference.
+  // 2. Otherwise, attempts to cache the key stored in sync prefernece.
+  //
+  // If kSharingDeriveVapidKey is disabled:
+  // 1. Attempts to cache the key stored in sync prefernece.
+  // 2. If failed and cahced key is absent, attempts to generate a new key. If
+  // successful, cache it and store in sync preference.
+  bool RefreshCachedKey();
+
  private:
+  // Attempts to update cached key if |new_key| is different from cached
+  // key, and store updated key to sync preferences. Returns true if cached
+  // key is updated.
+  bool UpdateCachedKey(std::unique_ptr<crypto::ECPrivateKey> new_key);
+
+  // Attempts to update cached key with key stored in sync preferences. Returns
+  // true if cached key is updated.
+  bool InitWithPreference();
+
   // Used for storing and fetching VAPID key from preferences.
   SharingSyncPreference* sharing_sync_preference_;
-
+  syncer::SyncService* sync_service_;
   std::unique_ptr<crypto::ECPrivateKey> vapid_key_;
+  std::vector<uint8_t> vapid_key_info_;
 
   DISALLOW_COPY_AND_ASSIGN(VapidKeyManager);
 };

chrome/browser/sharing/vapid_key_manager_unittest.cc

@@ -4,11 +4,13 @@
 
 #include "chrome/browser/sharing/vapid_key_manager.h"
 
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/sharing/features.h"
 #include "chrome/browser/sharing/sharing_sync_preference.h"
+#include "components/sync/driver/test_sync_service.h"
 #include "components/sync_device_info/fake_device_info_sync_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "crypto/ec_private_key.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -17,30 +19,98 @@ class VapidKeyManagerTest : public testing::Test {
  protected:
   VapidKeyManagerTest()
       : sharing_sync_preference_(&prefs_, &fake_device_info_sync_service_),
-        vapid_key_manager_(&sharing_sync_preference_) {
+        vapid_key_manager_(&sharing_sync_preference_, &test_sync_service_) {
     SharingSyncPreference::RegisterProfilePrefs(prefs_.registry());
   }
 
+  base::test::ScopedFeatureList scoped_feature_list_;
   sync_preferences::TestingPrefServiceSyncable prefs_;
   syncer::FakeDeviceInfoSyncService fake_device_info_sync_service_;
   SharingSyncPreference sharing_sync_preference_;
+  syncer::TestSyncService test_sync_service_;
   VapidKeyManager vapid_key_manager_;
 };
 
 }  // namespace
 
-TEST_F(VapidKeyManagerTest, GetOrCreateKey) {
+TEST_F(VapidKeyManagerTest, CreateKeyFlow) {
+  scoped_feature_list_.InitAndDisableFeature(kSharingDeriveVapidKey);
+
   // No keys stored in preferences.
   EXPECT_EQ(base::nullopt, sharing_sync_preference_.GetVapidKey());
 
   // Expected to create new keys and store in preferences.
-  crypto::ECPrivateKey* key = vapid_key_manager_.GetOrCreateKey();
-  std::vector<uint8_t> stored_key;
-  EXPECT_TRUE(key->ExportPrivateKey(&stored_key));
-  EXPECT_EQ(stored_key, sharing_sync_preference_.GetVapidKey());
-
-  // Expected to return old keys from preferences.
-  std::vector<uint8_t> temp_key;
-  EXPECT_TRUE(vapid_key_manager_.GetOrCreateKey()->ExportPrivateKey(&temp_key));
-  EXPECT_EQ(temp_key, stored_key);
+  crypto::ECPrivateKey* key_1 = vapid_key_manager_.GetOrCreateKey();
+  EXPECT_TRUE(key_1);
+  std::vector<uint8_t> key_info;
+  EXPECT_TRUE(key_1->ExportPrivateKey(&key_info));
+  EXPECT_EQ(key_info, sharing_sync_preference_.GetVapidKey());
+
+  // Expected to return same key when called again.
+  crypto::ECPrivateKey* key_2 = vapid_key_manager_.GetOrCreateKey();
+  EXPECT_TRUE(key_2);
+  std::vector<uint8_t> key_info_2;
+  EXPECT_TRUE(key_2->ExportPrivateKey(&key_info_2));
+  EXPECT_EQ(key_info, key_info_2);
+}
+
+TEST_F(VapidKeyManagerTest, ReadFromPreferenceFlow) {
+  scoped_feature_list_.InitAndDisableFeature(kSharingDeriveVapidKey);
+
+  // VAPID key already stored in preferences.
+  auto preference_key_1 = crypto::ECPrivateKey::Create();
+  ASSERT_TRUE(preference_key_1);
+  std::vector<uint8_t> preference_key_info_1;
+  ASSERT_TRUE(preference_key_1->ExportPrivateKey(&preference_key_info_1));
+  sharing_sync_preference_.SetVapidKey(preference_key_info_1);
+
+  // Expected to return key stored in preferences.
+  crypto::ECPrivateKey* key_1 = vapid_key_manager_.GetOrCreateKey();
+  EXPECT_TRUE(key_1);
+  std::vector<uint8_t> key_info_1;
+  EXPECT_TRUE(key_1->ExportPrivateKey(&key_info_1));
+  EXPECT_EQ(preference_key_info_1, key_info_1);
+
+  // Change VAPID key in sync prefernece.
+  auto preference_key_2 = crypto::ECPrivateKey::Create();
+  ASSERT_TRUE(preference_key_2);
+  std::vector<uint8_t> preference_key_info_2;
+  ASSERT_TRUE(preference_key_2->ExportPrivateKey(&preference_key_info_2));
+  sharing_sync_preference_.SetVapidKey(preference_key_info_2);
+
+  // Refresh local cache with new key in sync preference.
+  EXPECT_TRUE(vapid_key_manager_.RefreshCachedKey());
+  crypto::ECPrivateKey* key_2 = vapid_key_manager_.GetOrCreateKey();
+  EXPECT_TRUE(key_2);
+  std::vector<uint8_t> key_info_2;
+  EXPECT_TRUE(key_2->ExportPrivateKey(&key_info_2));
+  EXPECT_EQ(preference_key_info_2, key_info_2);
+}
+
+TEST_F(VapidKeyManagerTest, DeriveKeyFlow) {
+  scoped_feature_list_.InitAndEnableFeature(kSharingDeriveVapidKey);
+  test_sync_service_.SetExperimentalAuthenticationKey(
+      crypto::ECPrivateKey::Create());
+
+  // No keys stored in preferences.
+  EXPECT_EQ(base::nullopt, sharing_sync_preference_.GetVapidKey());
+
+  // Expected to derive key from sync secret and store in sync preferences.
+  crypto::ECPrivateKey* key_1 = vapid_key_manager_.GetOrCreateKey();
+  EXPECT_TRUE(key_1);
+  std::vector<uint8_t> key_info_1;
+  EXPECT_TRUE(key_1->ExportPrivateKey(&key_info_1));
+  EXPECT_EQ(key_info_1, sharing_sync_preference_.GetVapidKey());
+
+  // Change sync secret.
+  test_sync_service_.SetExperimentalAuthenticationKey(
+      crypto::ECPrivateKey::Create());
+
+  // Refresh local cache with new sync secret.
+  EXPECT_TRUE(vapid_key_manager_.RefreshCachedKey());
+  crypto::ECPrivateKey* key_2 = vapid_key_manager_.GetOrCreateKey();
+  EXPECT_TRUE(key_2);
+  std::vector<uint8_t> key_info_2;
+  EXPECT_TRUE(key_2->ExportPrivateKey(&key_info_2));
+  EXPECT_NE(key_info_1, key_info_2);
 }

components/sync/driver/test_sync_service.cc

@@ -4,6 +4,7 @@
 
 #include "components/sync/driver/test_sync_service.h"
 
+#include <utility>
 #include <vector>
 
 #include "base/time/time.h"
@@ -98,6 +99,11 @@ void TestSyncService::SetUserDemographics(
   user_demographics_result_ = user_demographics_result;
 }
 
+void TestSyncService::SetExperimentalAuthenticationKey(
+    std::unique_ptr<crypto::ECPrivateKey> experimental_authentication_key) {
+  experimental_authentication_key_ = std::move(experimental_authentication_key);
+}
+
 void TestSyncService::SetEmptyLastCycleSnapshot() {
   SetLastCycleSnapshot(SyncCycleSnapshot());
 }
@@ -130,6 +136,11 @@ void TestSyncService::FireStateChanged() {
     observer.OnStateChanged(this);
 }
 
+void TestSyncService::FireSyncCycleCompleted() {
+  for (auto& observer : observers_)
+    observer.OnSyncCycleCompleted(this);
+}
+
 SyncUserSettings* TestSyncService::GetUserSettings() {
   return &user_settings_;
 }
@@ -173,7 +184,10 @@ bool TestSyncService::RequiresClientUpgrade() const {
 
 std::unique_ptr<crypto::ECPrivateKey>
 TestSyncService::GetExperimentalAuthenticationKey() const {
-  return nullptr;
+  if (!experimental_authentication_key_)
+    return nullptr;
+
+  return experimental_authentication_key_->Copy();
 }
 
 std::unique_ptr<SyncSetupInProgressHandle>

components/sync/driver/test_sync_service.h

@@ -40,6 +40,8 @@ class TestSyncService : public SyncService {
   void SetLastCycleSnapshot(const SyncCycleSnapshot& snapshot);
   void SetUserDemographics(
       const UserDemographicsResult& user_demographics_result);
+  void SetExperimentalAuthenticationKey(
+      std::unique_ptr<crypto::ECPrivateKey> experimental_authentication_key);
 
   // Convenience versions of the above, for when the caller doesn't care about
   // the particular values in the snapshot, just whether there is one.
@@ -51,6 +53,7 @@ class TestSyncService : public SyncService {
   void SetIsUsingSecondaryPassphrase(bool enabled);
 
   void FireStateChanged();
+  void FireSyncCycleCompleted();
 
   // SyncService implementation.
   syncer::SyncUserSettings* GetUserSettings() override;
@@ -133,6 +136,8 @@ class TestSyncService : public SyncService {
 
   UserDemographicsResult user_demographics_result_;
 
+  std::unique_ptr<crypto::ECPrivateKey> experimental_authentication_key_;
+
   DISALLOW_COPY_AND_ASSIGN(TestSyncService);
 };