Fix parent access code validation for reauth

This change addresses two problems with parent access code
on login screen.
1. When doing reauth we should validate parent code against
all configurations that are available on device. We enforce
parent code when the device is owned by child, event if it
is different user going through reauth.
It will fix https://crbug.com/1140310.
2. IsDeviceOwnedByChild is causing a crash when the device
owner cannot be found on user list. While I cannot reproduce
such situation, it seems that either stored device owner id
is incorrect or we fail at reading users list from Local State.
Relaxing the owner check and defaulting to false will prevent
crash while we investigate the problem further.
It is a speculative fix for https://crbug.com/1140357.

Bug: 1140310, 1140357

Change-Id: I3d4afab92741d90cae768f58b0da831f0c6c163a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490855Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Denis Kuznetsov [CET] <antrim@chromium.org>
Commit-Queue: Aga Wronska <agawronska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822273}

ash/child_accounts/parent_access_controller_impl.cc

@@ -9,8 +9,10 @@
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/bind.h"
+#include "base/check.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string16.h"
+#include "components/session_manager/session_manager_types.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace ash {
@@ -171,6 +173,16 @@ bool ParentAccessControllerImpl::ShowWidget(
   if (PinRequestWidget::Get())
     return false;
 
+  // When there is no logged in user we should accept parent access code for any
+  // of child account added to the device.
+  const auto session_state =
+      Shell::Get()->session_controller()->GetSessionState();
+  const bool user_in_session =
+      session_state == session_manager::SessionState::LOGGED_IN_NOT_ACTIVE ||
+      session_state == session_manager::SessionState::ACTIVE ||
+      session_state == session_manager::SessionState::LOCKED;
+  DCHECK(user_in_session || child_account_id.empty());
+
   account_id_ = child_account_id;
   action_ = action;
   validation_time_ = validation_time;

ash/child_accounts/parent_access_controller_impl_unittest.cc

@@ -12,7 +12,10 @@
 #include "ash/login/ui/views_utils.h"
 #include "ash/public/cpp/child_accounts/parent_access_controller.h"
 #include "base/bind.h"
+#include "base/dcheck_is_on.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "components/account_id/account_id.h"
+#include "components/session_manager/session_manager_types.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/views/controls/button/label_button.h"
 
@@ -22,10 +25,13 @@ namespace {
 
 using ::testing::_;
 
+AccountId GetChildAccountId() {
+  return AccountId::FromUserEmail("child@gmail.com");
+}
+
 class ParentAccessControllerImplTest : public LoginTestBase {
  protected:
-  ParentAccessControllerImplTest()
-      : account_id_(AccountId::FromUserEmail("child@gmail.com")) {}
+  ParentAccessControllerImplTest() : account_id_(GetChildAccountId()) {}
   ~ParentAccessControllerImplTest() override = default;
 
   // LoginScreenTest:
@@ -55,11 +61,23 @@ class ParentAccessControllerImplTest : public LoginTestBase {
     access_granted ? ++successful_validation_ : ++back_action_;
   }
 
-  void StartParentAccess(
-      SupervisedAction action = SupervisedAction::kUnlockTimeLimits) {
+  // Starts parent access validation.
+  // Use this overloaded method if session state and supervised action are not
+  // relevant.
+  void StartParentAccess() {
+    GetSessionControllerClient()->SetSessionState(
+        session_manager::SessionState::LOCKED);
+    StartParentAccess(account_id_, SupervisedAction::kUnlockTimeLimits);
+  }
+
+  // Starts parent access validation with supervised |action|.
+  // Session state should be configured accordingly to the |action|.
+  void StartParentAccess(SupervisedAction action) {
     StartParentAccess(account_id_, action);
   }
 
+  // Starts parent access validation with supervised |action| and |account_id|.
+  // Session state should be configured accordingly to the |action|.
   void StartParentAccess(const AccountId& account_id, SupervisedAction action) {
     validation_time_ = base::Time::Now();
     ash::ParentAccessController::Get()->ShowWidget(
@@ -137,6 +155,8 @@ TEST_F(ParentAccessControllerImplTest, ParentAccessDialogFocus) {
 
 // Tests correct UMA reporting for parent access.
 TEST_F(ParentAccessControllerImplTest, ParentAccessUMARecording) {
+  GetSessionControllerClient()->SetSessionState(
+      session_manager::SessionState::LOCKED);
   StartParentAccess(SupervisedAction::kUnlockTimeLimits);
   histogram_tester_.ExpectBucketCount(
       ParentAccessControllerImpl::kUMAParentAccessCodeUsage,
@@ -145,6 +165,8 @@ TEST_F(ParentAccessControllerImplTest, ParentAccessUMARecording) {
   ExpectUMAActionReported(
       ParentAccessControllerImpl::UMAAction::kCanceledByUser, 1, 1);
 
+  GetSessionControllerClient()->SetSessionState(
+      session_manager::SessionState::ACTIVE);
   StartParentAccess(SupervisedAction::kUpdateTimezone);
   histogram_tester_.ExpectBucketCount(
       ParentAccessControllerImpl::kUMAParentAccessCodeUsage,
@@ -153,7 +175,6 @@ TEST_F(ParentAccessControllerImplTest, ParentAccessUMARecording) {
   ExpectUMAActionReported(
       ParentAccessControllerImpl::UMAAction::kCanceledByUser, 2, 2);
 
-  // The below usage depends on the session state.
   GetSessionControllerClient()->SetSessionState(
       session_manager::SessionState::ACTIVE);
   StartParentAccess(SupervisedAction::kUpdateClock);
@@ -166,7 +187,7 @@ TEST_F(ParentAccessControllerImplTest, ParentAccessUMARecording) {
 
   GetSessionControllerClient()->SetSessionState(
       session_manager::SessionState::LOGIN_PRIMARY);
-  StartParentAccess(SupervisedAction::kUpdateClock);
+  StartParentAccess(EmptyAccountId(), SupervisedAction::kUpdateClock);
   histogram_tester_.ExpectBucketCount(
       ParentAccessControllerImpl::kUMAParentAccessCodeUsage,
       ParentAccessControllerImpl::UMAUsage::kTimeChangeLoginScreen, 1);
@@ -186,7 +207,7 @@ TEST_F(ParentAccessControllerImplTest, ParentAccessUMARecording) {
 
   GetSessionControllerClient()->SetSessionState(
       session_manager::SessionState::LOGIN_PRIMARY);
-  StartParentAccess(SupervisedAction::kReauth);
+  StartParentAccess(EmptyAccountId(), SupervisedAction::kReauth);
   histogram_tester_.ExpectBucketCount(
       ParentAccessControllerImpl::kUMAParentAccessCodeUsage,
       ParentAccessControllerImpl::UMAUsage::kReauhLoginScreen, 1);
@@ -238,5 +259,26 @@ TEST_F(ParentAccessControllerImplTest, ParentAccessUnsuccessfulValidation) {
       ParentAccessControllerImpl::UMAAction::kCanceledByUser, 1, 3);
 }
 
+#if DCHECK_IS_ON()
+// Tests that on login screen we check parent access code against all accounts.
+TEST_F(ParentAccessControllerImplTest, EnforceNoAccountSpecifiedOnLogin) {
+  GetSessionControllerClient()->SetSessionState(
+      session_manager::SessionState::LOGIN_PRIMARY);
+  EXPECT_DEATH_IF_SUPPORTED(
+      StartParentAccess(GetChildAccountId(), SupervisedAction::kReauth), "");
+
+  GetSessionControllerClient()->SetSessionState(
+      session_manager::SessionState::LOGIN_PRIMARY);
+  EXPECT_DEATH_IF_SUPPORTED(
+      StartParentAccess(GetChildAccountId(), SupervisedAction::kAddUser), "");
+
+  GetSessionControllerClient()->SetSessionState(
+      session_manager::SessionState::LOGIN_PRIMARY);
+  EXPECT_DEATH_IF_SUPPORTED(
+      StartParentAccess(GetChildAccountId(), SupervisedAction::kUpdateClock),
+      "");
+}
+#endif
+
 }  // namespace
 }  // namespace ash

chrome/browser/chromeos/child_accounts/parent_access_code/parent_access_service.cc

@@ -9,6 +9,7 @@
 
 #include "ash/public/cpp/child_accounts/parent_access_controller.h"
 #include "base/check.h"
+#include "base/logging.h"
 #include "base/no_destructor.h"
 #include "base/timer/timer.h"
 #include "chrome/common/chrome_features.h"
@@ -26,13 +27,30 @@ using ash::SupervisedAction;
 
 // Returns true when the device owner is a child.
 bool IsDeviceOwnedByChild() {
+  // TODO(crbug.com/1143369): Owner id might not be available early after
+  // startup. Wait for it to be ready.
   AccountId owner_account_id =
       user_manager::UserManager::Get()->GetOwnerAccountId();
-  if (owner_account_id.empty())
+  if (owner_account_id.empty()) {
+    LOG(ERROR) << "Device owner could not be determined - will skip parent "
+                  "code validation";
     return false;
+  }
+
   const user_manager::User* device_owner =
       user_manager::UserManager::Get()->FindUser(owner_account_id);
-  CHECK(device_owner);
+
+  // It looks like reading users from Local State might be failing sometimes.
+  // Default to false if ownership is not known to avoid crash.
+  // TODO(agawronska): Investigate if it can be improved. Defaulting to false
+  // could sometimes lead to skipping parent code validation when child is the
+  // device owner.
+  if (!device_owner) {
+    LOG(ERROR) << "Device owner could not be determined - will skip parent "
+                  "code validation";
+    return false;
+  }
+
   return device_owner->IsChild();
 }
 

chrome/browser/ui/ash/login_screen_client.cc

@@ -182,8 +182,11 @@ void LoginScreenClient::ShowGaiaSignin(const AccountId& prefilled_account) {
           supervised_action)) {
     // Show the client native parent access widget and processed to GAIA signin
     // flow in |OnParentAccessValidation| when validation success.
+    // On login screen we want to validate parent access code for the
+    // device owner. Device owner might be different than the account that
+    // requires reauth, so we are passing an empty |account_id|.
     ash::ParentAccessController::Get()->ShowWidget(
-        prefilled_account,
+        AccountId(),
         base::BindOnce(&LoginScreenClient::OnParentAccessValidation,
                        weak_ptr_factory_.GetWeakPtr(), prefilled_account),
         supervised_action, false /* extra_dimmer */, base::Time::Now());