Various supervised user password fixes - 2

chrome/browser/chromeos/login/managed/supervised_user_authentication.cc

 // Copyright 2013 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 "chrome/browser/chromeos/login/managed/supervised_user_authentication.h"
 
 #include "base/base64.h"
 #include "base/command_line.h"
 #include "base/json/json_file_value_serializer.h"
 #include "base/metrics/histogram.h"
@@ skipping 84 matching lines @@
   std::string raw_result(out_bytes, out_bytes + sizeof(out_bytes));
 
   std::string result;
   base::Base64Encode(raw_result, &result);
   return result;
 }
 
 base::DictionaryValue* LoadPasswordData(base::FilePath profile_dir) {
   JSONFileValueSerializer serializer(profile_dir.Append(kPasswordUpdateFile));
   std::string error_message;
-  int error_code;
+  int error_code = JSONFileValueSerializer::JSON_NO_ERROR;
   scoped_ptr<base::Value> value(
       serializer.Deserialize(&error_code, &error_message));
   if (JSONFileValueSerializer::JSON_NO_ERROR != error_code) {
+    LOG(ERROR) << "Could not deserialize password data, error = " << error_code
+               << " / " << error_message;
     return NULL;
   }
   base::DictionaryValue* result;
   if (!value->GetAsDictionary(&result)) {
+    NOTREACHED();

[Bernhard Bauer, 2014/04/02 14:05:28]

In general, having a NOTREACHED() is an indication that you are handling a
DCHECK failure, which you're not supposed to do (see
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-,
which I also mentioned in your last CL).

In this particular case, you log an error instead of DCHECKing if the file can't
be deserialized at all above, and you also handle a NULL return value in the
caller, so you could just remove the NOTREACHED. If you want to, you could add a
log statement instead, if not, you could simply initialize |result| to NULL and
return it unconditionally (if the value is not a dictionary, |result| will stay
NULL, which is exactly what you want to return).

[Denis Kuznetsov (DE-MUC), 2014/04/02 14:48:51]

Done.

     return NULL;
   }
-  value.Pass();
+  // Ignore unused result warning.

[Nikita (slow), 2014/04/02 14:18:20]

nit: Add empty line before, place comment into {}
or just remove comment and use ignore_result() as Bernhard is suggesting.

[Denis Kuznetsov (DE-MUC), 2014/04/02 14:48:51]

Done.

+  if (value.release()) {

[Bernhard Bauer, 2014/04/02 14:05:28]

base/macros.h has ignore_result().

[Denis Kuznetsov (DE-MUC), 2014/04/02 14:48:51]

Done.

+  }
   return result;
 }
 
 void OnPasswordDataLoaded(
     const SupervisedUserAuthentication::PasswordDataCallback& success_callback,
     const base::Closure& failure_callback,
     base::DictionaryValue* value) {
   if (!value) {
     failure_callback.Run();
     return;
@@ skipping 80 matching lines @@
     password_data->SetStringWithoutPathExpansion(kSalt, salt);
     int revision = kMinPasswordRevision;
     password_data->SetIntegerWithoutPathExpansion(kPasswordRevision, revision);
     std::string salted_password =
         BuildPasswordForHashWithSaltSchema(salt, password);
     std::string base64_signature_key = BuildRawHMACKey();
     std::string base64_signature =
         BuildPasswordSignature(salted_password, revision, base64_signature_key);
     password_data->SetStringWithoutPathExpansion(kEncryptedPassword,
                                                  salted_password);
+    password_data->SetStringWithoutPathExpansion(kPasswordSignature,
+                                                 base64_signature);
 
     extra_data->SetStringWithoutPathExpansion(kPasswordEncryptionKey,
                                               BuildRawHMACKey());
     extra_data->SetStringWithoutPathExpansion(kPasswordSignatureKey,
                                               base64_signature_key);
     return true;
   }
   NOTREACHED();
   return false;
 }
@@ skipping 33 matching lines @@
   if (holder.GetIntegerWithoutPathExpansion(kSchemaVersion,
                                             &schema_version_index)) {
     schema_version = static_cast<Schema>(schema_version_index);
   }
   return schema_version;
 }
 
 bool SupervisedUserAuthentication::NeedPasswordChange(
     const std::string& user_id,
     const base::DictionaryValue* password_data) {
-
   base::DictionaryValue local;
   owner_->GetPasswordInformation(user_id, &local);
   int local_schema = SCHEMA_PLAIN;
   int local_revision = kMinPasswordRevision;
   int updated_schema = SCHEMA_PLAIN;
   int updated_revision = kMinPasswordRevision;
   local.GetIntegerWithoutPathExpansion(kSchemaVersion, &local_schema);
   local.GetIntegerWithoutPathExpansion(kPasswordRevision, &local_revision);
   password_data->GetIntegerWithoutPathExpansion(kSchemaVersion,
                                                 &updated_schema);
   password_data->GetIntegerWithoutPathExpansion(kPasswordRevision,
                                                 &updated_revision);
+  LOG(ERROR) << "-----5";

[Bernhard Bauer, 2014/04/02 14:05:28]

Please remove.

[Denis Kuznetsov (DE-MUC), 2014/04/02 14:48:51]

Done.

   if (updated_schema > local_schema)
     return true;
   DCHECK_EQ(updated_schema, local_schema);
   return updated_revision > local_revision;
 }
 
 void SupervisedUserAuthentication::ScheduleSupervisedPasswordChange(
     const std::string& supervised_user_id,
     const base::DictionaryValue* password_data) {
   const User* user = UserManager::Get()->FindUser(supervised_user_id);
@@ skipping 34 matching lines @@
 
 bool SupervisedUserAuthentication::HasIncompleteKey(
     const std::string& user_id) {
   base::DictionaryValue holder;
   owner_->GetPasswordInformation(user_id, &holder);
   bool incomplete_key = false;
   holder.GetBoolean(kHasIncompleteKey, &incomplete_key);
   return incomplete_key;
 }
 
-void SupervisedUserAuthentication::MarkKeyIncomplete(
-    const std::string& user_id) {
+void SupervisedUserAuthentication::MarkKeyIncomplete(const std::string& user_id,
+                                                     bool incomplete) {
   base::DictionaryValue holder;
   owner_->GetPasswordInformation(user_id, &holder);
-  holder.SetBoolean(kHasIncompleteKey, true);
+  holder.SetBoolean(kHasIncompleteKey, incomplete);
   owner_->SetPasswordInformation(user_id, &holder);
 }
 
 void SupervisedUserAuthentication::LoadPasswordUpdateData(
     const std::string& user_id,
     const PasswordDataCallback& success_callback,
     const base::Closure& failure_callback) {
   const User* user = UserManager::Get()->FindUser(user_id);
   base::FilePath profile_path =
       ProfileHelper::GetProfilePathByUserIdHash(user->username_hash());
   PostTaskAndReplyWithResult(
       content::BrowserThread::GetBlockingPool(),
       FROM_HERE,
       base::Bind(&LoadPasswordData, profile_path),
       base::Bind(&OnPasswordDataLoaded, success_callback, failure_callback));
 }
 
 }  // namespace chromeos