Google OAuth Device Flow support for FNL

services/authentication/accounts_db_manager.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/authentication/accounts_db_manager.h"
+
+#include <vector>
+
+#include "base/logging.h"
+#include "base/strings/string_tokenizer.h"
+#include "mojo/public/cpp/bindings/array.h"
+#include "mojo/public/cpp/bindings/type_converter.h"
+#include "mojo/services/files/interfaces/files.mojom.h"
+#include "services/authentication/authentication_impl_db.mojom.h"
+
+namespace authentication {
+
+char kAccountsDbFileName[] = "creds_db";
+char kAuthDbFileName[] = "auth_db";
+const uint32 kAuthDbVersion = 1;
+
+AccountsDbManager::AccountsDbManager()
+    : creds_db_file_(nullptr), auth_db_file_(nullptr), contents_(nullptr) {}
+
+AccountsDbManager::AccountsDbManager(
+    const mojo::files::DirectoryPtr directory) {
+  // Initialize in-memory contents from existing DB file
+  directory->OpenFile(
+      kAccountsDbFileName, GetProxy(&creds_db_file_),
+      mojo::files::kOpenFlagCreate | mojo::files::kOpenFlagRead |
+          mojo::files::kOpenFlagWrite,
+      [this](mojo::files::Error error) {
+        if (mojo::files::Error::OK != error) {
+          LOG(ERROR) << "Open() error on credentials db:" << error;
+          delete this;
+          return;
+        }
+      });
+  directory->OpenFile(kAuthDbFileName, GetProxy(&auth_db_file_),
+                      mojo::files::kOpenFlagCreate |
+                          mojo::files::kOpenFlagRead |
+                          mojo::files::kOpenFlagWrite,
+                      [this](mojo::files::Error error) {
+                        if (mojo::files::Error::OK != error) {
+                          LOG(ERROR) << "Open() error on auth db:" << error;
+                          delete this;
+                          return;
+                        }
+                      });
+  Initialize();
+}
+
+AccountsDbManager::~AccountsDbManager() {}
+
+mojo::String AccountsDbManager::GetUpdatedDbContents(
+    const mojo::String& username,
+    const mojo::String& new_account_data,
+    bool user_exists) {
+  if (username.is_null() || new_account_data.is_null()) {
+    return nullptr;
+  }
+  std::string buffer;
+  if (user_exists) {
+    // Remove existing user record
+    base::StringTokenizer lines(contents_, "\n");
+    std::string user_record;
+    while (lines.GetNext()) {

[qsr, 2016/02/16 14:17:06]

Please comment that you are removing the entry for user if it is present.

[ukode, 2016/02/26 21:35:49]

Had the same comment two lines above. Moved it down here.

+      user_record = lines.token();
+      std::size_t pos = user_record.find(username.get() + ",");
+      if ((pos == std::string::npos) || (pos != 0)) {

[qsr, 2016/02/16 14:17:06]

std::string::npos will not be 0, so pos != 0 is enough.

[ukode, 2016/02/26 21:35:49]

Acknowledged.

+        buffer += user_record;
+        buffer += "\n";
+      }
+    }
+  } else {
+    if (!contents_.empty()) {  // first account
+      buffer += "\n";
+    }
+  }
+  buffer += new_account_data;
+  return mojo::String(buffer);
+}
+
+void AccountsDbManager::UpdateAccount(const mojo::String& username,
+                                      const mojo::String& new_account_data) {
+  if (username.is_null() || new_account_data.is_null()) {
+    return;
+  }
+  bool user_exists = !GetAccountDataForUser(username).is_null();

[qsr, 2016/02/16 14:17:06]

This is parsing the string.

[ukode, 2016/02/26 21:35:49]

Acknowledged.

+
+  mojo::String new_db_contents =
+      GetUpdatedDbContents(username, new_account_data, user_exists);

[qsr, 2016/02/16 14:17:06]

And this is doing it again.

Any reason the in-memory format is not a map. Wouldn't it be better to just
tranform it as a string before saving? I have the impression you still need to
do all the string manipulation anyway, but at least, with a map, in memory
manipulation will be more efficient.

If you want to still be able to send partial file update, you might want to have
both representation.

[ukode, 2016/02/26 21:35:49]

Got rid of this extra parsing completely,and directly serializing the mojom
credentials map object. The file will get rewritten, but I assume in the interim
it will be hardly 1-10 accounts per device, which is not a lot of disk IO. 

+  if (new_db_contents.is_null()) {
+    return;
+  }
+
+  // Update contents cache with new data
+  if (user_exists) {
+    contents_.assign(new_db_contents.data(), new_db_contents.size());
+  } else {
+    contents_ += new_db_contents;
+  }
+
+  // Write updated contents to file
+  std::vector<uint8_t> bytes_to_write(new_db_contents.get().begin(),
+                                      new_db_contents.get().end());
+  bytes_to_write.push_back('\0');
+  mojo::files::Whence whence;
+  if (user_exists) {
+    whence = mojo::files::Whence::FROM_START;
+  } else {
+    whence = mojo::files::Whence::FROM_CURRENT;
+  }
+  creds_db_file_->Write(
+      mojo::Array<uint8_t>::From(bytes_to_write), 0, whence,
+      [this](mojo::files::Error error, uint32_t num_bytes_written) {
+        this->OnWriteResponse(error, num_bytes_written);
+      });
+}
+
+void AccountsDbManager::OnWriteResponse(const mojo::files::Error error,
+                                        const uint32_t num_bytes_written) {
+  if (mojo::files::Error::OK != error) {
+    LOG(ERROR) << "Write() error on accounts db:" << error;
+    delete this;

[qsr, 2016/02/16 14:17:06]

Return after the delete. Doesn't matter that much here, but if you ever add
something after the if, you will crash.

[ukode, 2016/02/26 21:35:49]

Done.

+  }
+  return;
+}
+
+mojo::String AccountsDbManager::GetAccountDataForUser(
+    const mojo::String& username) {
+  if (username.is_null() || contents_.empty()) {
+    return nullptr;
+  }
+
+  base::StringTokenizer lines(contents_, "\n");
+  std::string entry;
+  while (lines.GetNext()) {
+    entry = lines.token();
+    std::size_t pos = entry.find(username.get() + ",");
+    if ((pos != std::string::npos) && (pos == 0)) {
+      return mojo::String(entry);
+    }
+  }
+  return nullptr;
+}
+
+mojo::String AccountsDbManager::GetAllUserAccounts() {
+  return mojo::String(contents_);
+}
+
+void AccountsDbManager::Initialize() {
+  auth_grants.version = kAuthDbVersion;
+
+  const size_t kMaxReadSize = 1 * 1024 * 1024;
+  mojo::Array<uint8_t> cred_bytes_read;
+  creds_db_file_->Read(

[qsr, 2016/02/16 14:17:06]

Don't you have a race in this class? What happen if your methods are called
before this call (and the next one) finishes.

[ukode, 2016/02/26 21:35:49]

The async reads happen very quickly and have enough time to complete before the
first read or write happens as the thread has to wait on user authorization on a
secondary device. 

Just to future-proof the code for not running into a race condition, I added a
simple flag(db_init_option_) that keeps track of the init state. Tested this
approach and it works fine. PTAL.  

+      kMaxReadSize - 1, 0, mojo::files::Whence::FROM_START,
+      [this](mojo::files::Error error, mojo::Array<uint8_t> cred_bytes_read) {
+        this->OnReadResponse(error, cred_bytes_read.Pass());
+      });
+
+  mojo::Array<uint8_t> auth_bytes_read;
+  auth_db_file_->Read(
+      kMaxReadSize - 1, 0, mojo::files::Whence::FROM_START,
+      [this](mojo::files::Error error, mojo::Array<uint8_t> auth_bytes_read) {
+        this->OnAuthFileReadResponse(error, auth_bytes_read.Pass());
+      });
+}
+
+void AccountsDbManager::OnReadResponse(const mojo::files::Error error,

[qsr, 2016/02/16 14:17:06]

Maybe rename this. Response is not that clear when you have 2 different reads.

[ukode, 2016/02/26 21:35:49]

Acknowledged.

+                                       const mojo::Array<uint8_t> bytes_read) {
+  if (error != mojo::files::Error::OK) {
+    LOG(ERROR) << "Read() error on accounts db: " << error;
+    delete this;
+    return;
+  }
+
+  if (bytes_read.size() != 0) {
+    contents_.assign(reinterpret_cast<const char*>(&bytes_read[0]),
+                     bytes_read.size());
+  }
+}
+
+void AccountsDbManager::OnAuthFileReadResponse(
+    const mojo::files::Error error,
+    const mojo::Array<uint8_t> bytes_read) {
+  if (error != mojo::files::Error::OK) {
+    LOG(ERROR) << "Read() error on auth db: " << error;
+    delete this;
+    return;
+  }
+
+  if (bytes_read.size() != 0) {
+    // Deserialize data from file
+    const char* p = reinterpret_cast<const char*>(&bytes_read[0]);

[qsr, 2016/02/16 14:17:06]

s/p/data/g?

[ukode, 2016/02/26 21:35:49]

Done.

+    auth_grants.Deserialize((void*)p);
+    CHECK_EQ(auth_grants.version, kAuthDbVersion);
+    CHECK(!auth_grants.last_selected_accounts.is_null());
+  }
+}
+
+mojo::String AccountsDbManager::GetAuthorizedUserForApp(mojo::String app_url) {
+  if (app_url.is_null()) {
+    return nullptr;
+  }
+  auto it = auth_grants.last_selected_accounts.find(app_url);
+  if (it == auth_grants.last_selected_accounts.end()) {
+    return nullptr;
+  }
+  return mojo::String(it.GetValue());
+}
+
+void AccountsDbManager::UpdateAuthorization(mojo::String app_url,
+                                            mojo::String username) {
+  if (app_url.is_null() || username.is_null()) {
+    return;
+  }
+  auth_grants.last_selected_accounts[app_url] = username;
+
+  size_t buf_size = auth_grants.GetSerializedSize();
+  char buf[buf_size];
+  MOJO_CHECK(auth_grants.Serialize(buf, buf_size));
+
+  std::vector<uint8_t> bytes_to_write(buf, buf + buf_size);
+  bytes_to_write.push_back('\0');

[qsr, 2016/02/16 14:17:06]

Why this?

[ukode, 2016/02/26 21:35:49]

The very first version of files service needed this for some reason. Can't
recollect now. I tried it by removing and it works fine.

+  mojo::files::Whence whence;
+  whence = mojo::files::Whence::FROM_START;
+
+  auth_db_file_->Write(
+      mojo::Array<uint8_t>::From(bytes_to_write), 0, whence,
+      [this](mojo::files::Error error, uint32_t num_bytes_written) {
+        this->OnAuthFileWriteResponse(error, num_bytes_written);
+      });
+}
+
+void AccountsDbManager::OnAuthFileWriteResponse(
+    const mojo::files::Error error,
+    const uint32_t num_bytes_written) {
+  if (mojo::files::Error::OK != error) {
+    LOG(ERROR) << "Write() error on auth db:" << error;
+    delete this;
+  }
+  return;
+}
+
+}  // namespace authentication