Merge lp:~azzar1/unity/fix-missing-entry-lockscreen into lp:unity

[Andrea Azzarone]

Refactor the way UserAuthenticator is created and passed around. Handle failures to create new threads and fallback to a "Switch to greeter..." button in case of failure.

[Marco Trevisan (Treviño) (3v1n0)]

Looks good, thanks

review: Approve

=== modified file 'lockscreen/KylinUserPromptView.cpp'
--- lockscreen/KylinUserPromptView.cpp	2016-03-31 05:57:11 +0000
+++ lockscreen/KylinUserPromptView.cpp	2017-09-15 13:37:27 +0000
@@ -79,9 +79,9 @@
 
 }
 
-KylinUserPromptView::KylinUserPromptView(session::Manager::Ptr const& session_manager)
-  : AbstractUserPromptView(session_manager)
-  , session_manager_(session_manager)
+KylinUserPromptView::KylinUserPromptView(session::Manager::Ptr const& session_manager,
+                                         UserAuthenticator::Ptr const& user_authenticator)
+  : AbstractUserPromptView(session_manager, user_authenticator)
   , username_(nullptr)
   , msg_layout_(nullptr)
   , prompt_layout_(nullptr)
@@ -90,25 +90,25 @@
   , avatar_(nullptr)
   , avatar_icon_file("")
 {
-    user_authenticator_.echo_on_requested.connect([this](std::string const& message, PromiseAuthCodePtr const& promise){
+    user_authenticator_->echo_on_requested.connect(sigc::track_obj([this](std::string const& message, PromiseAuthCodePtr const& promise){
         AddPrompt(message, true, promise);
-    });
+    }, *this));
 
-    user_authenticator_.echo_off_requested.connect([this](std::string const& message, PromiseAuthCodePtr const& promise){
+    user_authenticator_->echo_off_requested.connect(sigc::track_obj([this](std::string const& message, PromiseAuthCodePtr const& promise){
         AddPrompt(message, false, promise);
-    });
+    }, *this));
 
-    user_authenticator_.message_requested.connect([this](std::string const& message){
+    user_authenticator_->message_requested.connect(sigc::track_obj([this](std::string const& message){
         AddMessage(message, nux::color::White);
-    });
+    }, *this));
 
-    user_authenticator_.error_requested.connect([this](std::string const& message){
+    user_authenticator_->error_requested.connect(sigc::track_obj([this](std::string const& message){
         AddMessage(message, nux::color::Red);
-    });
+    }, *this));
 
-    user_authenticator_.clear_prompts.connect([this](){
+    user_authenticator_->clear_prompts.connect(sigc::track_obj([this](){
         ResetLayout();
-    });
+    }, *this));
 
     scale.changed.connect(sigc::hide(sigc::mem_fun(this, &KylinUserPromptView::UpdateSize)));
 
@@ -121,7 +121,7 @@
     ResetLayout();
 
     TextureCache::GetDefault().themed_invalidated.connect(sigc::mem_fun(this, &KylinUserPromptView::ResetLayout));
-    user_authenticator_.AuthenticateStart(session_manager_->UserName(),
+    user_authenticator_->AuthenticateStart(session_manager_->UserName(),
                                           sigc::mem_fun(this, &KylinUserPromptView::AuthenticationCb));
 }
 
@@ -244,7 +244,7 @@
   {
     AddMessage(_("Invalid password, please try again"), nux::color::Red);
 
-    user_authenticator_.AuthenticateStart(session_manager_->UserName(),
+    user_authenticator_->AuthenticateStart(session_manager_->UserName(),
                                           sigc::mem_fun(this, &KylinUserPromptView::AuthenticationCb));
   }
 }

=== modified file 'lockscreen/KylinUserPromptView.h'
--- lockscreen/KylinUserPromptView.h	2016-03-16 10:47:16 +0000
+++ lockscreen/KylinUserPromptView.h	2017-09-15 13:37:27 +0000
@@ -44,7 +44,8 @@
 class KylinUserPromptView : public AbstractUserPromptView
 {
 public:
-  KylinUserPromptView(session::Manager::Ptr const& session_manager);
+  KylinUserPromptView(session::Manager::Ptr const& session_manager,
+                      UserAuthenticator::Ptr const& user_authenticator);
 
   nux::View* focus_view();
 
@@ -62,8 +63,6 @@
   nux::ObjectPtr<nux::BaseTexture> LoadUserIcon(std::string const& icon_file, int icon_size);
 
 private:
-  session::Manager::Ptr session_manager_;
-  UserAuthenticatorPam user_authenticator_;
   StaticCairoText* username_;
   nux::VLayout* msg_layout_;
   nux::VLayout* prompt_layout_;

=== modified file 'lockscreen/LockScreenAbstractPromptView.h'
--- lockscreen/LockScreenAbstractPromptView.h	2016-03-31 09:51:33 +0000
+++ lockscreen/LockScreenAbstractPromptView.h	2017-09-15 13:37:27 +0000
@@ -29,7 +29,7 @@
 #include <Nux/VLayout.h>
 #include "UnityCore/SessionManager.h"
 
-#include "UserAuthenticatorPam.h"
+#include "UserAuthenticator.h"
 #include "unity-shared/IMTextEntry.h"
 
 namespace nux
@@ -48,10 +48,12 @@
 class AbstractUserPromptView : public nux::View
 {
 public:
-  AbstractUserPromptView(session::Manager::Ptr const& session_manager)
+  AbstractUserPromptView(session::Manager::Ptr const& session_manager,
+                         UserAuthenticator::Ptr const& user_authenticator)
     : nux::View(NUX_TRACKER_LOCATION)
     , scale(1.0)
     , session_manager_(session_manager)
+    , user_authenticator_(user_authenticator)
   {}
 
   nux::Property<double> scale;
@@ -64,7 +66,7 @@
 
 protected:
   session::Manager::Ptr session_manager_;
-  UserAuthenticatorPam user_authenticator_;
+  UserAuthenticator::Ptr user_authenticator_;
   std::shared_ptr<nux::AbstractPaintLayer> bg_layer_;
   StaticCairoText* username_;
   nux::VLayout* msg_layout_;

=== modified file 'lockscreen/LockScreenController.cpp'
--- lockscreen/LockScreenController.cpp	2016-11-11 16:00:53 +0000
+++ lockscreen/LockScreenController.cpp	2017-09-15 13:37:27 +0000
@@ -27,6 +27,7 @@
 #include "LockScreenPromptFactory.h"
 #include "LockScreenShield.h"
 #include "LockScreenSettings.h"
+#include "UserAuthenticatorPam.h"
 #include "unity-shared/AnimationUtils.h"
 #include "unity-shared/InputMonitor.h"
 #include "unity-shared/UnitySettings.h"
@@ -70,6 +71,7 @@
   , upstart_wrapper_(upstart_wrapper)
   , shield_factory_(shield_factory)
   , suspend_inhibitor_manager_(std::make_shared<SuspendInhibitorManager>())
+  , user_authenticator_(std::make_shared<UserAuthenticatorPam>())
   , fade_animator_(unity::Settings::Instance().low_gfx() ? 0 : LOCK_FADE_DURATION)
   , blank_window_animator_(IDLE_FADE_DURATION)
   , test_mode_(test_mode)
@@ -259,7 +261,7 @@
 
   if (!prompt_view)
   {
-    prompt_view = test_mode_ ? nux::ObjectPtr<AbstractUserPromptView>() : PromptFactory::CreatePrompt(session_manager_);
+    prompt_view = test_mode_ ? nux::ObjectPtr<AbstractUserPromptView>() : PromptFactory::CreatePrompt(session_manager_, user_authenticator_);
     prompt_view_ = prompt_view.GetPointer();
   }
 

=== modified file 'lockscreen/LockScreenController.h'
--- lockscreen/LockScreenController.h	2016-11-11 16:00:53 +0000
+++ lockscreen/LockScreenController.h	2017-09-15 13:37:27 +0000
@@ -29,6 +29,7 @@
 #include "LockScreenAcceleratorController.h"
 #include "SuspendInhibitorManager.h"
 #include "ScreenSaverDBusManager.h"
+#include "UserAuthenticator.h"
 #include "unity-shared/BackgroundEffectHelper.h"
 #include "unity-shared/SystemdWrapper.h"
 #include "unity-shared/UpstartWrapper.h"
@@ -95,6 +96,7 @@
   UpstartWrapper::Ptr upstart_wrapper_;
   ShieldFactoryInterface::Ptr shield_factory_;
   SuspendInhibitorManager::Ptr suspend_inhibitor_manager_;
+  UserAuthenticator::Ptr user_authenticator_;
 
   nux::animation::AnimateValue<double> fade_animator_;
   nux::animation::AnimateValue<double> blank_window_animator_;

=== modified file 'lockscreen/LockScreenPromptFactory.cpp'
--- lockscreen/LockScreenPromptFactory.cpp	2015-12-04 08:17:46 +0000
+++ lockscreen/LockScreenPromptFactory.cpp	2017-09-15 13:37:27 +0000
@@ -26,14 +26,15 @@
 {
 namespace lockscreen
 {
-nux::ObjectPtr<AbstractUserPromptView> PromptFactory::CreatePrompt(session::Manager::Ptr const& sm)
+nux::ObjectPtr<AbstractUserPromptView> PromptFactory::CreatePrompt(session::Manager::Ptr const& sm,
+                                                                   UserAuthenticator::Ptr const& ua)
 {
   nux::ObjectPtr<AbstractUserPromptView> prompt;
 
   if (unity::Settings::Instance().desktop_type() == DesktopType::UBUNTUKYLIN)
-    prompt = new KylinUserPromptView(sm);
+    prompt = new KylinUserPromptView(sm, ua);
   else
-    prompt = new UserPromptView(sm);
+    prompt = new UserPromptView(sm, ua);
 
   return prompt;
 }

=== modified file 'lockscreen/LockScreenPromptFactory.h'
--- lockscreen/LockScreenPromptFactory.h	2016-03-31 09:51:33 +0000
+++ lockscreen/LockScreenPromptFactory.h	2017-09-15 13:37:27 +0000
@@ -22,6 +22,7 @@
 
 #include <NuxCore/NuxCore.h>
 #include "UnityCore/SessionManager.h"
+#include "UserAuthenticator.h"
 
 namespace unity
 {
@@ -33,7 +34,8 @@
 
 struct PromptFactory
 {
-  static nux::ObjectPtr<AbstractUserPromptView> CreatePrompt(session::Manager::Ptr const&);
+  static nux::ObjectPtr<AbstractUserPromptView> CreatePrompt(session::Manager::Ptr const&,
+                                                             UserAuthenticator::Ptr const&);
 };
 
 }

=== modified file 'lockscreen/UserAuthenticator.h'
--- lockscreen/UserAuthenticator.h	2014-03-05 04:09:13 +0000
+++ lockscreen/UserAuthenticator.h	2017-09-15 13:37:27 +0000
@@ -36,6 +36,7 @@
 class UserAuthenticator
 {
 public:
+  typedef std::shared_ptr<UserAuthenticator> Ptr;
   typedef std::function<void(bool)> AuthenticateEndCallback;
 
   virtual ~UserAuthenticator() = default;
@@ -43,6 +44,7 @@
   // Authenticate the user in a background thread.
   virtual bool AuthenticateStart(std::string const& username, AuthenticateEndCallback const&) = 0;
 
+  sigc::signal<void> start_failed;
   sigc::signal<void, std::string, PromiseAuthCodePtr const&> echo_on_requested;
   sigc::signal<void, std::string, PromiseAuthCodePtr const&> echo_off_requested;
   sigc::signal<void, std::string> message_requested;

=== modified file 'lockscreen/UserAuthenticatorPam.cpp'
--- lockscreen/UserAuthenticatorPam.cpp	2016-08-10 08:56:27 +0000
+++ lockscreen/UserAuthenticatorPam.cpp	2017-09-15 13:37:27 +0000
@@ -23,6 +23,7 @@
 
 #include "UserAuthenticatorPam.h"
 #include "unity-shared/UnitySettings.h"
+#include "UnityCore/GLibWrapper.h"
 
 #include <cstring>
 #include <security/pam_appl.h>
@@ -36,42 +37,49 @@
 bool UserAuthenticatorPam::AuthenticateStart(std::string const& username,
                                              AuthenticateEndCallback const& authenticate_cb)
 {
+  if (pam_handle_)
+    return false;
+
   first_prompt_ = true;
   username_ = username;
   authenticate_cb_ = authenticate_cb;
-  pam_handle_ = nullptr;
-
-  if (!InitPam() || !pam_handle_)
-    return false;
-
-  glib::Object<GTask> task(g_task_new(nullptr, cancellable_, [] (GObject*, GAsyncResult*, gpointer data) {
-    auto self = static_cast<UserAuthenticatorPam*>(data);
-    pam_end(self->pam_handle_, self->status_);
-    self->authenticate_cb_(self->status_ == PAM_SUCCESS);
-  }, this));
-
-  g_task_set_task_data(task, this, nullptr);
-
-  g_task_run_in_thread(task, [] (GTask* task, gpointer, gpointer data, GCancellable*) {
-    auto self = static_cast<UserAuthenticatorPam*>(data);
-
-    self->status_ = pam_authenticate(self->pam_handle_, 0);
-
-    if (self->status_ == PAM_SUCCESS)
-    {
-      int status2 = pam_acct_mgmt(self->pam_handle_, 0);
-
-      if (status2 == PAM_NEW_AUTHTOK_REQD)
-        status2 = pam_chauthtok(self->pam_handle_, PAM_CHANGE_EXPIRED_AUTHTOK);
-
-      if (unity::Settings::Instance().pam_check_account_type())
-        self->status_ = status2;
-
-      pam_setcred(self->pam_handle_, PAM_REINITIALIZE_CRED);
-    }
-  });
-
-  return true;
+
+  glib::Error error;
+  g_thread_try_new(nullptr, AuthenticationThreadFunc, this, &error);
+
+  return !error;
+}
+
+gpointer UserAuthenticatorPam::AuthenticationThreadFunc(gpointer data)
+{
+  auto self = static_cast<UserAuthenticatorPam*>(data);
+
+  if (!self->InitPam() || !self->pam_handle_)
+  {
+    self->pam_handle_ = nullptr;
+    self->source_manager_.AddTimeout(0, [self] { self->start_failed.emit(); return false; });
+    return nullptr;
+  }
+
+  self->status_ = pam_authenticate(self->pam_handle_, 0);
+
+  if (self->status_ == PAM_SUCCESS)
+  {
+    int status2 = pam_acct_mgmt(self->pam_handle_, 0);
+
+    if (status2 == PAM_NEW_AUTHTOK_REQD)
+      status2 = pam_chauthtok(self->pam_handle_, PAM_CHANGE_EXPIRED_AUTHTOK);
+
+    if (unity::Settings::Instance().pam_check_account_type())
+      self->status_ = status2;
+
+    pam_setcred(self->pam_handle_, PAM_REINITIALIZE_CRED);
+  }
+
+  pam_end(self->pam_handle_, self->status_);
+  self->pam_handle_ = nullptr;
+  self->source_manager_.AddTimeout(0, [self] {   self->authenticate_cb_(self->status_ == PAM_SUCCESS); return false; });
+  return nullptr;
 }
 
 bool UserAuthenticatorPam::InitPam()

=== modified file 'lockscreen/UserAuthenticatorPam.h'
--- lockscreen/UserAuthenticatorPam.h	2014-03-05 04:09:13 +0000
+++ lockscreen/UserAuthenticatorPam.h	2017-09-15 13:37:27 +0000
@@ -20,8 +20,6 @@
 #ifndef UNITY_USER_AUTHENTICATOR_PAM_H
 #define UNITY_USER_AUTHENTICATOR_PAM_H
 
-#include <boost/noncopyable.hpp>
-#include <UnityCore/GLibWrapper.h>
 #include <UnityCore/GLibSource.h>
 
 #include "UserAuthenticator.h"
@@ -36,13 +34,17 @@
 namespace lockscreen
 {
 
-class UserAuthenticatorPam : public UserAuthenticator, private boost::noncopyable
+class UserAuthenticatorPam : public UserAuthenticator
 {
 public:
+  UserAuthenticatorPam() = default;
   bool AuthenticateStart(std::string const& username, AuthenticateEndCallback const&) override;
 
 private:
-  // TODO (andy) move to pimpl
+  UserAuthenticatorPam(UserAuthenticatorPam const&) = delete;
+  UserAuthenticatorPam& operator=(UserAuthenticatorPam const&) = delete;
+
+  static gpointer AuthenticationThreadFunc(gpointer);
   bool InitPam();
 
   static int ConversationFunction(int num_msg,
@@ -53,10 +55,9 @@
   std::string username_;
   AuthenticateEndCallback authenticate_cb_;
 
-  int status_;
-  bool first_prompt_;
-  pam_handle* pam_handle_;
-  glib::Cancellable cancellable_;
+  int status_ = 0;
+  bool first_prompt_ = true;
+  pam_handle* pam_handle_ = nullptr;
   glib::SourceManager source_manager_;
 };
 

=== modified file 'lockscreen/UserPromptView.cpp'
--- lockscreen/UserPromptView.cpp	2016-06-23 14:51:42 +0000
+++ lockscreen/UserPromptView.cpp	2017-09-15 13:37:27 +0000
@@ -23,6 +23,7 @@
 #include <glib/gi18n-lib.h>
 
 #include <boost/algorithm/string/trim.hpp>
+#include <NuxCore/Logger.h>
 #include <Nux/VLayout.h>
 
 #include "LockScreenSettings.h"
@@ -38,6 +39,9 @@
 {
 namespace
 {
+
+DECLARE_LOGGER(logger, "unity.lockscreen");
+
 const RawPixel PADDING              = 10_em;
 const RawPixel LAYOUT_MARGIN        = 10_em;
 const RawPixel MSG_LAYOUT_MARGIN    = 15_em;
@@ -101,41 +105,46 @@
 
 }
 
-UserPromptView::UserPromptView(session::Manager::Ptr const& session_manager)
-  : AbstractUserPromptView(session_manager)
-  , session_manager_(session_manager)
+UserPromptView::UserPromptView(session::Manager::Ptr const& session_manager,
+                               UserAuthenticator::Ptr const& user_authenticator)
+  : AbstractUserPromptView(session_manager, user_authenticator)
   , username_(nullptr)
   , msg_layout_(nullptr)
   , prompt_layout_(nullptr)
   , button_layout_(nullptr)
   , prompted_(false)
   , unacknowledged_messages_(false)
+  , num_retry_auth_(0)
 {
-  user_authenticator_.echo_on_requested.connect([this](std::string const& message, PromiseAuthCodePtr const& promise){
+  user_authenticator_->start_failed.connect(sigc::track_obj([this](){
+    HandleAuthenticationStartFailure();
+  }, *this));
+
+  user_authenticator_->echo_on_requested.connect(sigc::track_obj([this](std::string const& message, PromiseAuthCodePtr const& promise){
     prompted_ = true;
     unacknowledged_messages_ = false;
     AddPrompt(message, /* visible */ true, promise);
-  });
+  }, *this));
 
-  user_authenticator_.echo_off_requested.connect([this](std::string const& message, PromiseAuthCodePtr const& promise){
+  user_authenticator_->echo_off_requested.connect(sigc::track_obj([this](std::string const& message, PromiseAuthCodePtr const& promise){
     prompted_ = true;
     unacknowledged_messages_ = false;
     AddPrompt(message, /* visible */ false, promise);
-  });
+  }, *this));
 
-  user_authenticator_.message_requested.connect([this](std::string const& message){
+  user_authenticator_->message_requested.connect(sigc::track_obj([this](std::string const& message){
     unacknowledged_messages_ = true;
     AddMessage(message, nux::color::White);
-  });
+  }, *this));
 
-  user_authenticator_.error_requested.connect([this](std::string const& message){
+  user_authenticator_->error_requested.connect(sigc::track_obj([this](std::string const& message){
     unacknowledged_messages_ = true;
     AddMessage(message, nux::color::Red);
-  });
+  }, *this));
 
-  user_authenticator_.clear_prompts.connect([this](){
+  user_authenticator_->clear_prompts.connect(sigc::track_obj([this](){
     ResetLayout();
-  });
+  }, *this));
 
   scale.changed.connect(sigc::hide(sigc::mem_fun(this, &UserPromptView::UpdateSize)));
 
@@ -469,8 +478,11 @@
   prompted_ = false;
   unacknowledged_messages_ = false;
 
-  user_authenticator_.AuthenticateStart(session_manager_->UserName(),
-                                        sigc::mem_fun(this, &UserPromptView::AuthenticationCb));
+  if(!user_authenticator_->AuthenticateStart(session_manager_->UserName(),
+                                             sigc::mem_fun(this, &UserPromptView::AuthenticationCb)))
+  {
+    HandleAuthenticationStartFailure();
+  }
 }
 
 void UserPromptView::DoUnlock()
@@ -478,5 +490,29 @@
   session_manager_->unlock_requested.emit();
 }
 
+void UserPromptView::HandleAuthenticationStartFailure()
+{
+  ++num_retry_auth_;
+
+  if (num_retry_auth_ <= 5)
+  {
+    LOG_WARNING(logger) << "Failed to start the authentication process. Retrying for " << num_retry_auth_ << " time.";
+    source_manager_.AddTimeout(100, [this] {
+      StartAuthentication();
+      return false;
+    });
+  }
+  else
+  {
+    num_retry_auth_ = 0;
+
+    AddMessage(_("Authentication failure"), nux::color::Red);
+    AddButton(_("Switch to greeter…"), [this] {
+      session_manager_->SwitchToGreeter();
+    });
+    GetLayout()->AddLayout(button_layout_);
+  }
+}
+
 }
 }

=== modified file 'lockscreen/UserPromptView.h'
--- lockscreen/UserPromptView.h	2016-06-23 14:51:42 +0000
+++ lockscreen/UserPromptView.h	2017-09-15 13:37:27 +0000
@@ -25,10 +25,10 @@
 
 #include <Nux/Nux.h>
 #include <Nux/View.h>
+#include "UnityCore/GLibSource.h"
 #include "UnityCore/SessionManager.h"
 
 #include "LockScreenAbstractPromptView.h"
-#include "UserAuthenticatorPam.h"
 #include "unity-shared/IMTextEntry.h"
 
 namespace nux
@@ -48,7 +48,8 @@
 class UserPromptView : public AbstractUserPromptView
 {
 public:
-  UserPromptView(session::Manager::Ptr const& session_manager);
+  UserPromptView(session::Manager::Ptr const& session_manager,
+                 UserAuthenticator::Ptr const& user_authenticator);
 
   nux::View* focus_view();
 
@@ -70,9 +71,8 @@
   void ShowAuthenticated(bool successful);
   void StartAuthentication();
   void DoUnlock();
+  void HandleAuthenticationStartFailure();
 
-  session::Manager::Ptr session_manager_;
-  UserAuthenticatorPam user_authenticator_;
   std::shared_ptr<nux::AbstractPaintLayer> bg_layer_;
   StaticCairoText* username_;
   nux::VLayout* msg_layout_;
@@ -84,6 +84,9 @@
 
   bool prompted_;
   bool unacknowledged_messages_;
+  int num_retry_auth_ = 0;
+
+  glib::SourceManager source_manager_;
 };
 
 }