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"
+
+namespace authentication {
+
+const char* kAccountsDbFileName = "accounts_db.txt";

[viettrungluu, 2016/01/11 21:23:44]

nit: This isn't as const as you think it is. You probably want |char
kAccountsDbFileName[] = "accounts_db.txt";|.

(Otherwise, |const char* const kAccountsDbFileName = ...;| would also be OK.)

[ukode, 2016/02/09 00:20:17]

Thanks for the tip. Done.

+
+AccountsDbManager::AccountsDbManager() : file_(nullptr), contents_(nullptr) {}
+
+AccountsDbManager::AccountsDbManager(mojo::files::DirectoryPtr& directory) {

[viettrungluu, 2016/01/11 21:23:45]

Depending on how this is used, this should either take a |DirectoryPtr| (not a
reference) -- if the caller doesn't want to hang on to it -- or a
|DirectoryPtr*|.

(In general, the Google C++ style forbids non-const references.)

[ukode, 2016/02/09 00:20:17]

Done.

+  // Open accounts db file
+  mojo::files::Error error = mojo::files::Error::INTERNAL;
+  directory.Pass()->OpenFile(kAccountsDbFileName, GetProxy(&file_),

[viettrungluu, 2016/01/11 21:23:44]

No |.Pass()| here.

[ukode, 2016/02/09 00:20:17]

Done.

+                             mojo::files::kOpenFlagCreate |
+                                 mojo::files::kOpenFlagRead |
+                                 mojo::files::kOpenFlagWrite,
+                             Capture(&error));
+  directory.WaitForIncomingResponse();

[viettrungluu, 2016/01/11 21:23:44]

Instead of waiting for the response (esp. since you just die anyway on failure),
maybe you should actually provide a callback that verifies success?

[ukode, 2016/02/09 00:20:17]

Done.

+  if (mojo::files::Error::OK != error) {
+    LOG(FATAL) << "Not able to open accounts database file.";
+  }
+}
+
+AccountsDbManager::~AccountsDbManager() {
+  // Close the accounts db file
+  mojo::files::Error error = mojo::files::Error::INTERNAL;
+  file_->Close(Capture(&error));
+  file_.WaitForIncomingResponse();

[viettrungluu, 2016/01/11 21:23:45]

Ditto here. Note that closing the file is optional. The file is effectively
closed if |file_| is destroyed. (The only extra utility is that calling
|Close()| gives some opportunity of reporting error, but it's not a big deal
since you just die anyway on failure.)

[ukode, 2016/02/09 00:20:17]

Ok,removing the file close functionality for now.

+  if (mojo::files::Error::OK != error) {
+    LOG(FATAL) << "Not able to close accounts database file.";
+  }
+}
+
+void AccountsDbManager::GetUpdatedDbContents(
+    const mojo::String& username,
+    const mojo::String& new_account_data,
+    bool user_exists,
+    mojo::String& new_db_contents) {

[viettrungluu, 2016/01/11 21:23:44]

For this method, you should probably just return a |mojo::String| (which may be
null on error).

[ukode, 2016/02/09 00:20:18]

Done.

+  if (username.is_null() || new_account_data.is_null()) {
+    return;
+  }
+  std::string buffer;
+  if (user_exists) {
+    // Remove existing user record
+    base::StringTokenizer lines(AccountsDbManager::contents_, "\n");

[viettrungluu, 2016/01/11 21:23:44]

(Here and below....) |AccountsDbManager::| not needed.

[ukode, 2016/02/09 00:20:18]

Done.

+    std::string user_record;
+    while (lines.GetNext()) {
+      user_record = lines.token();
+      if (user_record.find(username) == std::string::npos) {

[viettrungluu, 2016/01/11 21:23:44]

What's the format of contents_? Does this properly handle the case when one
username is a substring of another?

(Is there other data for which this may be problematic?)

In general, I'm not sure why you don't store the data in memory in an
already-parsed way, and only serialize/deserialize when writing to/reading from
the file.

[ukode, 2016/02/09 00:20:17]

Good catch. Added necessary checks and removed file operations for the most part
except during initialization and any new commits.

+        buffer += user_record;
+        buffer += "\n";
+      }
+    }
+  } else {
+    if (!AccountsDbManager::contents_.empty()) {  // first account
+      buffer += "\n";

[viettrungluu, 2016/01/11 21:23:45]

Why is this needed? (It results in |buffer| beginning with a newline.)

[ukode, 2016/02/09 00:20:17]

This is the line separator between records in the file. If there are no contents
to begin with then a newline is omitted through this check (never added to
buffer).

+    }
+  }
+  buffer += new_account_data;
+  new_db_contents = buffer;
+  return;

[viettrungluu, 2016/01/11 21:23:44]

nit: no |return| needed here

[ukode, 2016/02/09 00:20:17]

Done.

+}
+
+bool AccountsDbManager::UpdateAccount(const mojo::String& username,
+                                      const mojo::String& new_account_data) {
+  if (username.is_null() || new_account_data.is_null()) {
+    return false;
+  }
+  bool user_exists =
+      (AccountsDbManager::contents_.find(username) != std::string::npos)
+          ? true

[viettrungluu, 2016/01/11 21:23:44]

|? true : false| is unnecessary.

[ukode, 2016/02/09 00:20:18]

Done.

+          : false;
+
+  mojo::String new_db_contents;
+  GetUpdatedDbContents(username, new_account_data, user_exists,
+                       new_db_contents);
+  if (new_db_contents.is_null()) {
+    return false;
+  }
+
+  // Rewrite the file each time with updated info.
+  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::Error error = mojo::files::Error::INTERNAL;
+  uint32_t num_bytes_written = 0;
+  mojo::files::Whence whence;
+  if (user_exists) {
+    whence = mojo::files::Whence::FROM_START;
+  } else {
+    whence = mojo::files::Whence::FROM_CURRENT;
+  }
+  file_->Write(mojo::Array<uint8_t>::From(bytes_to_write), 0, whence,
+               Capture(&error, &num_bytes_written));
+  file_.WaitForIncomingResponse();

[viettrungluu, 2016/01/11 21:23:44]

You shouldn't do a blocking write like this. In general, you should handle this
asynchronously, which obviously requires propagating the asynchronousness to the
callers of this method as well.

I.e., you should start the request here and then pass a callback.

Note that doing a write synchronously here will bring all request processing to
a halt (since all requests are processed on your main thread).

(There are alternatives to doing things asynchronously, but would require, e.g.,
handling requests on worker threads or something like that.)

[ukode, 2016/02/09 00:20:17]

Acknowledged. MOved to async method.

+  if (mojo::files::Error::OK != error) {
+    return false;
+  }
+
+  // Update the contents cache with new data
+  if (user_exists) {
+    AccountsDbManager::contents_.assign(new_db_contents.data(),

[viettrungluu, 2016/01/11 21:23:44]

Keeping contents_ in sync with the actual contents may become more difficult if
you make things async.

That said, depending on the failure mode of Write(), contents_ may already be
wrong.

So you could just optimistically update contents_ and assume that Write()
succeeds.

[ukode, 2016/02/09 00:20:18]

Acknowledged.

+                                        new_db_contents.size());
+  } else {
+    AccountsDbManager::contents_ += new_db_contents;
+  }
+  return true;
+}
+
+void AccountsDbManager::GetAccountDataForUser(const mojo::String& username,
+                                              mojo::String& user_data) {
+  if (username.is_null()) {
+    return;
+  }
+
+  if (AccountsDbManager::contents_.empty()) {
+    mojo::Array<uint8_t> all_accounts_data(
+        AccountsDbManager::FetchAllAccounts());
+    if (!all_accounts_data.size()) {
+      return;
+    }
+  }
+
+  base::StringTokenizer lines(AccountsDbManager::contents_, "\n");
+  std::string entry;
+  while (lines.GetNext()) {
+    entry = lines.token();
+    if (entry.find(username) != std::string::npos) {
+      user_data.Swap(&entry);
+      return;
+    }
+  }
+}
+
+mojo::Array<uint8_t> AccountsDbManager::FetchAllAccounts() {
+  const size_t kMaxReadSize = 1 * 1024 * 1024;
+  mojo::Array<uint8_t> bytes_read;
+
+  mojo::files::Error error = mojo::files::Error::INTERNAL;
+  file_->Read(kMaxReadSize - 1, 0, mojo::files::Whence::FROM_START,
+              Capture(&error, &bytes_read));
+  file_.WaitForIncomingResponse();
+  if (mojo::files::Error::OK != error) {
+    return mojo::Array<uint8_t>();
+  }
+
+  const std::vector<uint8_t> vec = bytes_read.storage();
+  AccountsDbManager::contents_.assign((char*)vec.data(), vec.size());
+
+  return bytes_read.Pass();
+}
+
+}  // namespace authentication