Chromad: Create AuthPolicyCredentialsManager

chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc

+// Copyright 2017 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/authpolicy/auth_policy_credentials_manager.h"
+
+#include "base/location.h"
+#include "base/memory/singleton.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/lifetime/application_lifetime.h"
+#include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_delegate.h"
+#include "chrome/browser/notifications/notification_ui_manager.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/grit/chromium_strings.h"
+#include "chrome/grit/generated_resources.h"
+#include "chrome/grit/theme_resources.h"
+#include "chromeos/dbus/auth_policy_client.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "content/public/browser/browser_thread.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+
+namespace {
+
+const base::TimeDelta kGetUserStatusCallsInterval =

[Thiemo Nagel, 2017/05/24 10:11:05]

Global objects must be constexpr.

[Roman Sorokin (ftl), 2017/05/26 12:03:36]

Done.

+    base::TimeDelta::FromHours(1);
+const char kProfileSigninNotificationId[] = "chrome://settings/signin/";
+
+// A notification delegate for the sign-out button.
+class SigninNotificationDelegate : public NotificationDelegate {
+ public:
+  explicit SigninNotificationDelegate(const std::string& id);
+
+  // NotificationDelegate:
+  void Click() override;
+  void ButtonClick(int button_index) override;
+  std::string id() const override;
+
+ protected:
+  ~SigninNotificationDelegate() override;

[Thiemo Nagel, 2017/05/24 10:11:05]

Are you sure this is necessary?  Why not just use the default destructor?

[Roman Sorokin (ftl), 2017/05/26 12:03:37]

Done.

+
+ private:
+  // Unique id of the notification.
+  const std::string id_;
+
+  DISALLOW_COPY_AND_ASSIGN(SigninNotificationDelegate);
+};
+
+SigninNotificationDelegate::SigninNotificationDelegate(const std::string& id)
+    : id_(id) {}
+
+SigninNotificationDelegate::~SigninNotificationDelegate() {}
+
+void SigninNotificationDelegate::Click() {
+  chrome::AttemptUserExit();
+}
+
+void SigninNotificationDelegate::ButtonClick(int button_index) {
+  chrome::AttemptUserExit();
+}
+
+std::string SigninNotificationDelegate::id() const {
+  return id_;
+}
+
+}  // namespace
+
+AuthPolicyCredentialsManager::AuthPolicyCredentialsManager(Profile* profile)
+    : profile_(profile), weak_factory_(this) {

[Thiemo Nagel, 2017/05/24 10:11:04]

Nit: For weak_factory_, I'd prefer in-class initialization:

... weak_factory_{this};

[Roman Sorokin (ftl), 2017/05/26 12:03:36]

Done.

+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

[Thiemo Nagel, 2017/05/24 10:11:04]

Nit: Any particular reason you're checking for this?  Somehow I can't imagine
someone trying to run this on a non-UI thread...

(Same below.)

[Roman Sorokin (ftl), 2017/05/26 12:03:36]

I dunno, just in case :)

[Thiemo Nagel, 2017/05/26 12:46:21]

What is your code doing that requires running on the UI thread?  If you're
calling into some UI-thread-specific code, shouldn't it be the job of that code
to check for UI thread?

Reason I keep going about this is that I'm afraid that enforcing specific
threads makes servicification harder which afaiu might bring about more
pervasive multithreading.  Thus imho we shouldn't enforce running on a specific
thread without good reason.

[Roman Sorokin (ftl), 2017/05/26 13:06:32]

make sense! thanks!

+  StartObserveNetwork();

[Thiemo Nagel, 2017/05/24 10:11:04]

Nit: I prefer to have CHECK()'s at the top of the function so that the
assertions are easily glanceable.  Could you please move StartObserveNetwork()
below the CHECK()?

[Roman Sorokin (ftl), 2017/05/26 12:03:36]

Done.

+  const user_manager::User* user =
+      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
+  CHECK(user && user->IsActiveDirectoryUser());
+  account_id_ = user->GetAccountId();
+  GetUserStatus();
+}
+
+AuthPolicyCredentialsManager::~AuthPolicyCredentialsManager() {}
+
+void AuthPolicyCredentialsManager::Shutdown() {
+  StopObserveNetwork();
+}
+
+void AuthPolicyCredentialsManager::DefaultNetworkChanged(
+    const chromeos::NetworkState* network) {
+  GetUserStatusIfConnected(network);
+}
+
+void AuthPolicyCredentialsManager::NetworkConnectionStateChanged(
+    const chromeos::NetworkState* network) {
+  GetUserStatusIfConnected(network);
+}
+
+void AuthPolicyCredentialsManager::OnShuttingDown() {
+  StopObserveNetwork();
+}
+
+void AuthPolicyCredentialsManager::GetUserStatus() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

[Thiemo Nagel, 2017/05/24 10:11:04]

Same as above.

+  DCHECK(!weak_factory_.HasWeakPtrs());
+  should_call_get_status_again_ = false;
+  scheduled_get_user_status_call_.Cancel();
+  chromeos::DBusThreadManager::Get()->GetAuthPolicyClient()->GetUserStatus(
+      account_id_.GetObjGuid(),
+      base::BindOnce(&AuthPolicyCredentialsManager::OnGetUserStatusCallback,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void AuthPolicyCredentialsManager::OnGetUserStatusCallback(
+    authpolicy::ErrorType error,
+    const authpolicy::ActiveDirectoryUserStatus& user_status) {
+  DCHECK(weak_factory_.HasWeakPtrs());
+  ScheduleGetUserStatus();
+  last_error_ = error;

[ljusten (tachyonic), 2017/05/22 16:52:00]

I believe this will call GetUserStatus() over and over again if
should_call_get_status_again_ is true and the calls keep failing. Do this
instead:
bool call_again = should_call_get_status_again_;
should_call_get_status_again_ = false;
...
  if (call_again)

[Thiemo Nagel, 2017/05/24 10:11:04]

Simply setting |should_call_get_status_again_| to false before the return should
be sufficient, imho.

[Roman Sorokin (ftl), 2017/05/26 12:03:36]

Done.

[Roman Sorokin (ftl), 2017/05/26 12:03:36]

Done.

+  if (error != authpolicy::ERROR_NONE) {
+    DLOG(ERROR) << "GetUserStatus failed with " << error;
+    if (should_call_get_status_again_)
+      GetUserStatus();
+    return;
+  }
+  CHECK(user_status.account_info().account_id() == account_id_.GetObjGuid());
+  should_call_get_status_again_ = false;
+  if (user_status.has_account_info())
+    UpdateDisplayAndGivenName(user_status.account_info());
+
+  DCHECK(user_status.has_password_status());
+  switch (user_status.password_status()) {
+    case authpolicy::ActiveDirectoryUserStatus::PASSWORD_VALID:
+      // do nothing
+      break;
+    case authpolicy::ActiveDirectoryUserStatus::PASSWORD_EXPIRED:
+      ShowNotification(IDS_ACTIVE_DIRECTORY_PASSWORD_EXPIRED);
+      return;
+    case authpolicy::ActiveDirectoryUserStatus::PASSWORD_CHANGED:
+      ShowNotification(IDS_ACTIVE_DIRECTORY_PASSWORD_CHANGED);
+      return;
+  }
+
+  DCHECK(user_status.has_tgt_status());
+  switch (user_status.tgt_status()) {
+    case authpolicy::ActiveDirectoryUserStatus::TGT_VALID:
+      // do nothing
+      break;
+    case authpolicy::ActiveDirectoryUserStatus::TGT_EXPIRED:
+    case authpolicy::ActiveDirectoryUserStatus::TGT_NOT_FOUND:
+      ShowNotification(IDS_ACTIVE_DIRECTORY_REFRESH_AUTH_TOKEN);
+      return;
+  }
+  // Everything is ok.
+  user_manager::UserManager::Get()->SaveForceOnlineSignin(account_id_, false);
+}
+
+void AuthPolicyCredentialsManager::ScheduleGetUserStatus() {
+  scheduled_get_user_status_call_.Reset(base::Bind(
+      &AuthPolicyCredentialsManager::GetUserStatus, base::Unretained(this)));

[Thiemo Nagel, 2017/05/24 10:11:04]

Why is unretained safe here?  Maybe add a comment?

[Roman Sorokin (ftl), 2017/05/26 12:03:36]

Done.

+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

[Thiemo Nagel, 2017/05/24 10:11:04]

Afaics, this does not re-schedule after wake from sleep (and thus the maximal
interval between two calls can be (sleep time + kGetUserStatusCallsInterval)). 
No need to fix this right away, but I think it would be good to include a TODO.

[Roman Sorokin (ftl), 2017/05/26 12:03:36]

Done.

+      FROM_HERE, scheduled_get_user_status_call_.callback(),
+      kGetUserStatusCallsInterval);
+}
+
+void AuthPolicyCredentialsManager::StartObserveNetwork() {
+  DCHECK(chromeos::NetworkHandler::IsInitialized());
+  chromeos::NetworkHandler::Get()->network_state_handler()->AddObserver(
+      this, FROM_HERE);
+}
+
+void AuthPolicyCredentialsManager::StopObserveNetwork() {
+  DCHECK(chromeos::NetworkHandler::IsInitialized());
+  chromeos::NetworkHandler::Get()->network_state_handler()->RemoveObserver(
+      this, FROM_HERE);
+}
+
+void AuthPolicyCredentialsManager::UpdateDisplayAndGivenName(
+    const authpolicy::ActiveDirectoryAccountInfo& account_info) {
+  if (display_name_ == account_info.display_name() &&
+      given_name_ == account_info.given_name()) {
+    return;
+  }
+  display_name_ = account_info.display_name();
+  given_name_ = account_info.given_name();
+  user_manager::UserManager::Get()->UpdateUserAccountData(
+      account_id_,
+      user_manager::UserManager::UserAccountData(
+          base::UTF8ToUTF16(display_name_), base::UTF8ToUTF16(given_name_),
+          std::string() /* locale */));
+}
+
+void AuthPolicyCredentialsManager::ShowNotification(int message_id) {
+  user_manager::UserManager::Get()->SaveForceOnlineSignin(account_id_, true);
+
+  if (was_notification_shown_)
+    return;
+
+  message_center::RichNotificationData data;
+  data.buttons.push_back(message_center::ButtonInfo(
+      l10n_util::GetStringUTF16(IDS_SYNC_RELOGIN_LINK_LABEL)));
+
+  const std::string notification_id =
+      kProfileSigninNotificationId + profile_->GetProfileUserName();
+  // Set the delegate for the notification's sign-out button.
+  SigninNotificationDelegate* delegate =
+      new SigninNotificationDelegate(notification_id);
+
+  message_center::NotifierId notifier_id(
+      message_center::NotifierId::SYSTEM_COMPONENT,
+      kProfileSigninNotificationId);
+
+  // Set |profile_id| for multi-user notification blocker.
+  notifier_id.profile_id = profile_->GetProfileUserName();
+
+  Notification notification(
+      message_center::NOTIFICATION_TYPE_SIMPLE,
+      l10n_util::GetStringUTF16(IDS_SIGNIN_ERROR_BUBBLE_VIEW_TITLE),
+      l10n_util::GetStringUTF16(message_id),
+      ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+          IDR_NOTIFICATION_ALERT),
+      notifier_id,
+      base::string16(),  // display_source
+      GURL(notification_id), notification_id, data, delegate);
+  notification.SetSystemPriority();
+
+  NotificationUIManager* notification_ui_manager =
+      g_browser_process->notification_ui_manager();
+  // Add the notification.
+  notification_ui_manager->Add(notification, profile_);
+  was_notification_shown_ = true;
+}
+
+void AuthPolicyCredentialsManager::GetUserStatusIfConnected(
+    const chromeos::NetworkState* network) {
+  if (!network || !network->IsConnectedState())
+    return;
+  if (weak_factory_.HasWeakPtrs()) {
+    // Another call is in progress.
+    should_call_get_status_again_ = true;
+    return;
+  }
+  if (last_error_ != authpolicy::ERROR_NONE)
+    GetUserStatus();
+}
+
+// static
+AuthPolicyCredentialsManagerFactory*
+AuthPolicyCredentialsManagerFactory::GetInstance() {
+  return base::Singleton<AuthPolicyCredentialsManagerFactory>::get();
+}
+
+// static
+void AuthPolicyCredentialsManagerFactory::BuildForProfileIfActiveDirectory(
+    Profile* profile) {
+  const user_manager::User* user =
+      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
+  if (!user || !user->IsActiveDirectoryUser())
+    return;
+  GetInstance()->GetServiceForBrowserContext(profile, true /* create */);
+}
+
+AuthPolicyCredentialsManagerFactory::AuthPolicyCredentialsManagerFactory()
+    : BrowserContextKeyedServiceFactory(
+          "AuthPolicyCredentialsManager",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+AuthPolicyCredentialsManagerFactory::~AuthPolicyCredentialsManagerFactory() {}
+
+KeyedService* AuthPolicyCredentialsManagerFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  Profile* profile = Profile::FromBrowserContext(context);
+  return new AuthPolicyCredentialsManager(profile);
+}