Various supervised user password fixes - 2

Various supervised user password fixes - 2
1) JSON loader does not set error code to 0 on success.
2) Store signature on user import
3) Do not filter out need password change/incomplete flags.

R=nkostylev@chromium.org
TBR=bauerb@chromium.org

BUG=244472
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261409

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

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
(right):

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc:139:
DCHECK_EQ(stage_,  STAGE_INITIAL);
Nit: expected value goes first.

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:115:
NOTREACHED();
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).

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:119: if
(value.release()) {
base/macros.h has ignore_result().

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:288:
LOG(ERROR) << "-----5";
Please remove.

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:290: &flag))
If the condition is more than one line, use braces for the body.

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/managed_m...
File chrome/browser/managed_mode/chromeos/manager_password_service.cc (right):

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:195: if
(auth->HasIncompleteKey(user_id)) {
From looking at the rest of the code in this file, leaving out braces for
single-line bodies seems more common.

[Nikita (slow), 2014-04-02 14:18:19]

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:118: //
Ignore unused result warning.
nit: Add empty line before, place comment into {}
or just remove comment and use ignore_result() as Bernhard is suggesting.

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc
(right):

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc:184:
auth->MarkKeyIncomplete(user_id(), true);
nit: true /* incomplete */

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc:233:
auth->MarkKeyIncomplete(user_id(), true);
nit: true /* incomplete */

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/managed_m...
File chrome/browser/managed_mode/chromeos/manager_password_service.cc (right):

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:195: if
(auth->HasIncompleteKey(user_id)) {
nit: drop {}

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:195: if
(auth->HasIncompleteKey(user_id)) {
nit: Please add comment that you're essentially marking this as complete now.

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:196:
auth->MarkKeyIncomplete(user_id, false);
nit: false /* incomplete */

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:198: // Check
if we have legacy labels for keys.
nit: Insert empty line.

https://codereview.chromium.org/221813006/diff/30001/chrome/browser/managed_m...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:199: //
TODO(antrim): migrate it to GetLabels call once wad@ implement it.
nit: Capitalize comment.

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

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
File
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
(right):

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc:139:
DCHECK_EQ(stage_,  STAGE_INITIAL);
On 2014/04/02 14:05:28, Bernhard Bauer wrote:
> Nit: expected value goes first.

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_authentication.cc
(right):

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:115:
NOTREACHED();
On 2014/04/02 14:05:28, Bernhard Bauer wrote:
> 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).

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:118: //
Ignore unused result warning.
On 2014/04/02 14:18:20, Nikita Kostylev wrote:
> nit: Add empty line before, place comment into {}
> or just remove comment and use ignore_result() as Bernhard is suggesting.

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:119: if
(value.release()) {
On 2014/04/02 14:05:28, Bernhard Bauer wrote:
> base/macros.h has ignore_result().

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_authentication.cc:288:
LOG(ERROR) << "-----5";
On 2014/04/02 14:05:28, Bernhard Bauer wrote:
> Please remove.

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
File chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc
(right):

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc:184:
auth->MarkKeyIncomplete(user_id(), true);
On 2014/04/02 14:18:20, Nikita Kostylev wrote:
> nit: true /* incomplete */

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
chrome/browser/chromeos/login/managed/supervised_user_login_flow.cc:233:
auth->MarkKeyIncomplete(user_id(), true);
On 2014/04/02 14:18:20, Nikita Kostylev wrote:
> nit: true /* incomplete */

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
File chrome/browser/chromeos/login/supervised_user_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ch...
chrome/browser/chromeos/login/supervised_user_manager_impl.cc:290: &flag))
On 2014/04/02 14:05:28, Bernhard Bauer wrote:
> If the condition is more than one line, use braces for the body.

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ma...
File chrome/browser/managed_mode/chromeos/manager_password_service.cc (right):

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ma...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:195: if
(auth->HasIncompleteKey(user_id)) {
On 2014/04/02 14:05:28, Bernhard Bauer wrote:
> From looking at the rest of the code in this file, leaving out braces for
> single-line bodies seems more common.

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ma...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:198: // Check
if we have legacy labels for keys.
On 2014/04/02 14:18:20, Nikita Kostylev wrote:
> nit: Insert empty line.

Done.

https://chromiumcodereview.appspot.com/221813006/diff/30001/chrome/browser/ma...
chrome/browser/managed_mode/chromeos/manager_password_service.cc:199: //
TODO(antrim): migrate it to GetLabels call once wad@ implement it.
On 2014/04/02 14:18:20, Nikita Kostylev wrote:
> nit: Capitalize comment.

Done.

[Bernhard Bauer, 2014-04-02 14:52:40]

lgtm

[Nikita (slow), 2014-04-02 15:09:29]

lgtm

[commit-bot: I haz the power, 2014-04-02 15:09:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/221813006/50001

[commit-bot: I haz the power, 2014-04-02 15:10:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 15:10:12]

Failed to apply patch for
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
  Hunk #2 FAILED at 249.
  Hunk #3 succeeded at 258 with fuzz 2 (offset -23 lines).
  Hunk #4 succeeded at 297 (offset -22 lines).
  Hunk #5 succeeded at 322 (offset -22 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc.rej

Patch:      
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
Index:
chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
diff --git
a/chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
b/chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
index
b9eaca0d5ca21b96777afc8890587119695e8264..06cf6f43d577930f00880470fe476c7875c868a5
100644
---
a/chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
+++
b/chrome/browser/chromeos/login/managed/managed_user_creation_controller_new.cc
@@ -136,7 +136,7 @@ void ManagedUserCreationControllerNew::StartImport(
 
 void ManagedUserCreationControllerNew::StartCreationImpl() {
   DCHECK(creation_context_);
-  DCHECK(stage_ == STAGE_INITIAL);
+  DCHECK_EQ(STAGE_INITIAL, stage_);
   VLOG(1) << "Starting supervised user creation";
   VLOG(1) << " Phase 1 : Prepare keys";
 
@@ -249,7 +249,7 @@ void
ManagedUserCreationControllerNew::OnAuthenticationFailure(
 void ManagedUserCreationControllerNew::OnMountSuccess(
     const std::string& mount_hash) {
   DCHECK(creation_context_);
-  DCHECK(stage_ == KEYS_GENERATED);
+  DCHECK_EQ(KEYS_GENERATED, stage_);
   VLOG(1) << " Phase 2.2 : Created home dir with master key";
 
   creation_context_->mount_hash = mount_hash;
@@ -281,7 +281,7 @@ void ManagedUserCreationControllerNew::OnMountSuccess(
 
 void ManagedUserCreationControllerNew::OnAddKeySuccess() {
   DCHECK(creation_context_);
-  DCHECK(stage_ == KEYS_GENERATED);
+  DCHECK_EQ(KEYS_GENERATED, stage_);
   stage_ = CRYPTOHOME_CREATED;
 
   VLOG(1) << " Phase 3 : Create/update user on chrome.com/manage";
@@ -319,7 +319,7 @@ void ManagedUserCreationControllerNew::RegistrationCallback(
     const GoogleServiceAuthError& error,
     const std::string& token) {
   DCHECK(creation_context_);
-  DCHECK(stage_ == CRYPTOHOME_CREATED);
+  DCHECK_EQ(CRYPTOHOME_CREATED, stage_);
 
   stage_ = DASHBOARD_CREATED;
 
@@ -344,7 +344,7 @@ void ManagedUserCreationControllerNew::RegistrationCallback(
 
 void ManagedUserCreationControllerNew::OnManagedUserFilesStored(bool success) {
   DCHECK(creation_context_);
-  DCHECK(stage_ == DASHBOARD_CREATED);
+  DCHECK_EQ(DASHBOARD_CREATED, stage_);
 
   if (!success) {
     stage_ = STAGE_ERROR;

[Denis Kuznetsov (DE-MUC), 2014-04-03 10:05:51]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-04-03 10:06:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/221813006/50001

[commit-bot: I haz the power, 2014-04-03 14:40:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-03 14:40:46]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Nikita (slow), 2014-04-03 14:43:58]

On 2014/04/03 14:40:46, I haz the power (commit-bot) wrote:
> Commit queue rejected this change because the description was changed
> between the time the change entered the commit queue and the time it
> was ready to commit. You can safely check the commit box again.

I'm filing a bug about this.

[Nikita (slow), 2014-04-03 14:47:22]

On 2014/04/03 14:43:58, Nikita Kostylev wrote:
> On 2014/04/03 14:40:46, I haz the power (commit-bot) wrote:
> > Commit queue rejected this change because the description was changed
> > between the time the change entered the commit queue and the time it
> > was ready to commit. You can safely check the commit box again.
> 
> I'm filing a bug about this.

https://code.google.com/p/chromium/issues/detail?id=359595

[Denis Kuznetsov (DE-MUC), 2014-04-03 15:17:41]

The CQ bit was checked by antrim@chromium.org

[commit-bot: I haz the power, 2014-04-03 15:17:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/antrim@chromium.org/221813006/50001

[commit-bot: I haz the power, 2014-04-03 15:26:06]

Change committed as 261409