Deliver key by Upload.

For all parameterized Storage tests added a new dimension:
encryption enabled/disabled - now the same
opening/writing/reading/reopending is tested with and without encryption.
Fixed few minor mistakes, found with the test.

Bug:b:170054326

Change-Id: I72df7e96b7163a5b1f7a3f43553b11ac73c15472
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587505
Commit-Queue: Leonid Baraz <lbaraz@chromium.org>
Reviewed-by: Zach Trudo <zatrudo@google.com>
Cr-Commit-Position: refs/heads/master@{#837717}

chrome/browser/policy/messaging_layer/encryption/encryption_module.cc

@@ -77,6 +77,13 @@ void EncryptionModule::EncryptRecord(
   }
 
   // Encryptor enabled: start encryption of the record as a whole.
+  if (!has_encryption_key()) {
+    // Encryption key is not available.
+    std::move(cb).Run(
+        Status(error::NOT_FOUND, "Cannot encrypt record - no key"));
+    return;
+  }
+  // Encryption key is available, encrypt.
   encryptor_->OpenRecord(base::BindOnce(
       [](base::StringPiece record,
          base::OnceCallback<void(StatusOr<EncryptedRecord>)> cb,

chrome/browser/policy/messaging_layer/encryption/test_encryption_module.cc

@@ -31,9 +31,8 @@ void TestEncryptionModuleStrict::UpdateAsymmetricKey(
     base::StringPiece new_public_key,
     Encryptor::PublicKeyId new_public_key_id,
     base::OnceCallback<void(Status)> response_cb) {
-  std::move(response_cb)
-      .Run(Status(error::UNIMPLEMENTED,
-                  "Test Encryption Module does not accept any keys"));
+  // Ignore keys but return success.
+  std::move(response_cb).Run(Status(Status::StatusOK()));
 }
 
 TestEncryptionModuleStrict::~TestEncryptionModuleStrict() = default;

chrome/browser/policy/messaging_layer/public/report_client.cc

@@ -101,6 +101,42 @@ const base::FilePath::CharType kReportingDirectory[] =
 
 }  // namespace
 
+// Uploader is passed to Storage in order to upload messages using the
+// UploadClient.
+class ReportingClient::Uploader : public Storage::UploaderInterface {
+ public:
+  using UploadCallback =
+      base::OnceCallback<Status(std::unique_ptr<std::vector<EncryptedRecord>>)>;
+
+  static StatusOr<std::unique_ptr<Uploader>> Create(
+      UploadCallback upload_callback);
+
+  ~Uploader() override;
+  Uploader(const Uploader& other) = delete;
+  Uploader& operator=(const Uploader& other) = delete;
+
+  void ProcessRecord(EncryptedRecord data,
+                     base::OnceCallback<void(bool)> processed_cb) override;
+  void ProcessGap(SequencingInformation start,
+                  uint64_t count,
+                  base::OnceCallback<void(bool)> processed_cb) override;
+
+  void Completed(bool need_encryption_key, Status final_status) override;
+
+ private:
+  explicit Uploader(UploadCallback upload_callback_);
+
+  static void RunUpload(
+      UploadCallback upload_callback,
+      std::unique_ptr<std::vector<EncryptedRecord>> encrypted_records);
+
+  UploadCallback upload_callback_;
+
+  bool completed_{false};
+  std::unique_ptr<std::vector<EncryptedRecord>> encrypted_records_;
+  scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
+};
+
 ReportingClient::Uploader::Uploader(UploadCallback upload_callback)
     : upload_callback_(std::move(upload_callback)),
       encrypted_records_(std::make_unique<std::vector<EncryptedRecord>>()),
@@ -161,7 +197,8 @@ void ReportingClient::Uploader::ProcessGap(
           std::move(processed_cb)));
 }
 
-void ReportingClient::Uploader::Completed(Status final_status) {
+void ReportingClient::Uploader::Completed(bool need_encryption_key,
+                                          Status final_status) {
   if (!final_status.ok()) {
     // No work to do - something went wrong with storage and it no longer wants
     // to upload the records. Let the records die with |this|.
@@ -174,6 +211,10 @@ void ReportingClient::Uploader::Completed(Status final_status) {
   }
   completed_ = true;
 
+  if (need_encryption_key) {
+    // Attach encryption key information.
+  }
+
   sequenced_task_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(&Uploader::RunUpload, std::move(upload_callback_),

chrome/browser/policy/messaging_layer/public/report_client.h

@@ -212,41 +212,7 @@ class ReportingClient {
       CreateReportQueueCallback create_cb);
 
  private:
-  // Uploader is passed to Storage in order to upload messages using the
-  // UploadClient.
-  class Uploader : public Storage::UploaderInterface {
-   public:
-    using UploadCallback = base::OnceCallback<Status(
-        std::unique_ptr<std::vector<EncryptedRecord>>)>;
-
-    static StatusOr<std::unique_ptr<Uploader>> Create(
-        UploadCallback upload_callback);
-
-    ~Uploader() override;
-    Uploader(const Uploader& other) = delete;
-    Uploader& operator=(const Uploader& other) = delete;
-
-    void ProcessRecord(EncryptedRecord data,
-                       base::OnceCallback<void(bool)> processed_cb) override;
-    void ProcessGap(SequencingInformation start,
-                    uint64_t count,
-                    base::OnceCallback<void(bool)> processed_cb) override;
-
-    void Completed(Status final_status) override;
-
-   private:
-    explicit Uploader(UploadCallback upload_callback_);
-
-    static void RunUpload(
-        UploadCallback upload_callback,
-        std::unique_ptr<std::vector<EncryptedRecord>> encrypted_records);
-
-    UploadCallback upload_callback_;
-
-    bool completed_{false};
-    std::unique_ptr<std::vector<EncryptedRecord>> encrypted_records_;
-    scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
-  };
+  class Uploader;
 
   // Holds the creation request for a ReportQueue.
   class CreateReportQueueRequest {

chrome/browser/policy/messaging_layer/storage/storage.cc

@@ -142,8 +142,12 @@ class Storage::QueueUploaderInterface : public StorageQueue::UploaderInterface {
                                    std::move(processed_cb));
   }
 
-  void Completed(Status final_status) override {
-    storage_interface_->Completed(final_status);
+  void Completed(bool need_encryption_key, Status final_status) override {
+    // TODO(b/170054326): Add periodic key request.
+    // if (time to update key) {
+    //   need_encryption_key = true;
+    // }
+    storage_interface_->Completed(need_encryption_key, final_status);
   }
 
  private:
@@ -276,8 +280,7 @@ class Storage::KeyInStorage {
         continue;
       }
       uint64_t file_index = 0;
-      bool success = base::StringToUint64(extension, &file_index);
-      if (!success) {
+      if (!base::StringToUint64(extension.substr(1), &file_index)) {
         // Bad extension - not a number. Should not happen, will remove this
         // file.
         continue;
@@ -317,7 +320,7 @@ class Storage::KeyInStorage {
         continue;
       }
       uint64_t file_index = 0;
-      bool success = base::StringToUint64(extension, &file_index);
+      bool success = base::StringToUint64(extension.substr(1), &file_index);
       if (!success) {
         // Bad extension - not a number. Should not happen (file is corrupt).
         continue;
@@ -452,12 +455,18 @@ void Storage::Create(
       } else {
         if (EncryptionModule::is_enabled()) {
           // Encryptor enabled - we cannot proceed with no keys.
-          // TODO(b/170054326): For now - bail out. Change it to allow the
-          // Storage to be initialized, send Upload with no records and
-          // need_encryption_key flag and reject Enqueues until it is
-          // responded.
-          Response(status);
-          return;
+          // Send Upload with need_encryption_key flag and no records.
+          StatusOr<std::unique_ptr<UploaderInterface>> uploader =
+              storage_->start_upload_cb_.Run(
+                  MANUAL_BATCH);  // Any priority would do.
+          if (!uploader.ok()) {
+            Response(uploader.status());
+            return;
+          }
+          uploader.ValueOrDie()->Completed(/*need_encryption_key=*/true,
+                                           Status::StatusOK());
+          // Continue initialization without waiting for it to respond.
+          // Until the response arrives, we will reject Enqueues.
         }
       }
 
@@ -565,9 +574,6 @@ Status Storage::Flush(Priority priority) {
 void Storage::UpdateEncryptionKey(SignedEncryptionInfo signed_encryption_key) {
   // TODO(b/170054326): Verify received key signature. Bail out if failed.
 
-  // TODO(b/170054326): Serialize whole signed_encryption_key to a new file,
-  // discard the old one.
-
   // Assign the received key to encryption module.
   encryption_module_->UpdateAsymmetricKey(
       signed_encryption_key.public_asymmetric_key(),

chrome/browser/policy/messaging_layer/storage/storage_queue.cc

@@ -664,7 +664,8 @@ class StorageQueue::ReadContext : public TaskRunnerContext<Status> {
               scoped_refptr<StorageQueue> storage_queue)
       : TaskRunnerContext<Status>(
             base::BindOnce(&UploaderInterface::Completed,
-                           base::Unretained(uploader.get())),
+                           base::Unretained(uploader.get()),
+                           /*need_encryption_key=*/false),
             storage_queue->sequenced_task_runner_),
         uploader_(std::move(uploader)),
         storage_queue_weakptr_factory_{storage_queue.get()} {

chrome/browser/policy/messaging_layer/storage/storage_queue.h

@@ -64,7 +64,8 @@ class StorageQueue : public base::RefCountedThreadSafe<StorageQueue> {
 
     // Finalizes the upload (e.g. sends the message to server and gets
     // response). Called always, regardless of whether there were errors.
-    virtual void Completed(Status final_status) = 0;
+    // Set |need_encryption_key| if key is needed (initially or periodically).
+    virtual void Completed(bool need_encryption_key, Status final_status) = 0;
   };
 
   // Callback type for UploadInterface provider for this queue.

chrome/browser/policy/messaging_layer/storage/storage_queue_unittest.cc

@@ -192,12 +192,14 @@ class MockUploadClient : public StorageQueue::UploaderInterface {
         .Run(UploadGap(sequencing_information.sequencing_id(), count));
   }
 
-  void Completed(Status status) override { UploadComplete(status); }
+  void Completed(bool need_encryption_key, Status status) override {
+    UploadComplete(need_encryption_key, status);
+  }
 
   MOCK_METHOD(bool, UploadRecord, (uint64_t, base::StringPiece), (const));
   MOCK_METHOD(bool, UploadRecordFailure, (uint64_t, Status), (const));
   MOCK_METHOD(bool, UploadGap, (uint64_t, uint64_t), (const));
-  MOCK_METHOD(void, UploadComplete, (Status), (const));
+  MOCK_METHOD(void, UploadComplete, (bool, Status), (const));
 
   // Helper class for setting up mock client expectations of a successful
   // completion.
@@ -205,7 +207,7 @@ class MockUploadClient : public StorageQueue::UploaderInterface {
    public:
     explicit SetUp(MockUploadClient* client) : client_(client) {}
     ~SetUp() {
-      EXPECT_CALL(*client_, UploadComplete(Eq(Status::StatusOK())))
+      EXPECT_CALL(*client_, UploadComplete(_, Eq(Status::StatusOK())))
           .Times(1)
           .InSequence(client_->test_upload_sequence_);
     }