Merge lp:~mterry/unity8/dismiss-old-pam-prompts into lp:unity8

[Michael Terry]

Allow logging into a desktop or tablet session again, by properly dismissing old PAM conversations. (LP: #1350878)

In a desktop or tablet, we were accidentally starting two PAM conversations in sequence on startup. Which is a small bug; it shouldn't normally be a problem, since each new PAM conversation should kill the old one.

But the way we were killing the old one was subject to a thread race condition. See, a PAM conversation thread won't exit until all its prompts are answered. And what we do when we kill a PAM conversation is to answer all prompts with empty strings.

But it's possible that when we want to kill a PAM conversation that it hasn't actually gotten to the point of prompting us yet. And when those prompts do come through, we were treating them as prompts for the new PAM conversation.

So I've changed the PAM conversation logic to include a pam_handle and compare the handle with the current handle when being prompted. If it's an old handle, we just dismiss the prompt with an empty string response.

Oh, and I fixed the bug that caused two prompts on startup in the first place. (But we still need the above logic anyway, for when you switch users quickly.)

Allow logging into a desktop or tablet session again, by properly dismissing old PAM conversations. (LP: #1350878)

In a desktop or tablet, we were accidentally starting two PAM conversations in sequence on startup. Which is a small bug; it shouldn't normally be a problem, since each new PAM conversation should kill the old one.

But the way we were killing the old one was subject to a thread race condition. See, a PAM conversation thread won't exit until all its prompts are answered. And what we do when we kill a PAM conversation is to answer all prompts with empty strings.

But it's possible that when we want to kill a PAM conversation that it hasn't actually gotten to the point of prompting us yet. And when those prompts do come through, we were treating them as prompts for the new PAM conversation.

So I've changed the PAM conversation logic to include a pam_handle and compare the handle with the current handle when being prompted. If it's an old handle, we just dismiss the prompt with an empty string response.

Oh, and I fixed the bug that caused two prompts on startup in the first place. (But we still need the above logic anyway, for when you switch users quickly.)

== Checklist ==

 * Are there any related MPs required for this MP to build/function as expected? Please list.
 - No

 * Did you perform an exploratory manual test run of your code change and any related functionality?
 - Yes

 * Did you make sure that your branch does not contain spurious tags?
 - Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
 - NA

 * If you changed the UI, has there been a design review?
 - NA

[Michael Terry (mterry)]

To test this, you can edit qml/Shell.qml to set "tablet: true" and then run "./run.sh --lightdm=demo -f" and try to log in with your desktop password.

Or you can install a unity8 desktop session and try to log into unity8. Or install Touch on a tablet and set a password.

[Stephen M. Webb (bregma)]

Works for me on the desktop.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1180
http://jenkins.qa.ubuntu.com/job/unity8-ci/4024/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/3775
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-utopic/1025
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-utopic-amd64-ci/1118
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-utopic-armhf-ci/1118
        deb: http://jenkins.qa.ubuntu.com/job/unity8-utopic-armhf-ci/1118/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-utopic-i386-ci/1118
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/3642
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5022
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5022/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/11744

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/4024/rebuild

review: Approve (continuous-integration)

[Michał Sawicz (saviq)]

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Stephen did :)

 * Did CI run pass? If not, please explain why.
Y

review: Approve

=== modified file 'qml/Greeter/GreeterContent.qml'
--- qml/Greeter/GreeterContent.qml	2014-06-11 15:36:51 +0000
+++ qml/Greeter/GreeterContent.qml	2014-08-19 13:44:40 +0000
@@ -87,7 +87,6 @@
 
         onLoaded: {
             item.currentIndex = greeterContentLoader.currentIndex;
-            item.resetAuthentication();
         }
 
         Binding {
 
=== modified file 'tests/mocks/LightDM/demo/GreeterPrivate.cpp'
--- tests/mocks/LightDM/demo/GreeterPrivate.cpp	2014-07-15 16:38:02 +0000
+++ tests/mocks/LightDM/demo/GreeterPrivate.cpp	2014-08-19 13:44:40 +0000
@@ -33,6 +33,12 @@
 {
     Q_OBJECT
 
+    struct AppData
+    {
+        GreeterImpl *impl;
+        pam_handle *handle;
+    };
+
     typedef QFutureInterface<QString> ResponseFuture;
 
 public:
@@ -46,11 +52,11 @@
 
         connect(&futureWatcher, SIGNAL(finished()),
                 this, SLOT(finishPam()));
-        connect(this, SIGNAL(showMessage(QString, QLightDM::Greeter::MessageType)),
-                greeter, SIGNAL(showMessage(QString, QLightDM::Greeter::MessageType)));
+        connect(this, SIGNAL(showMessage(pam_handle *, QString, QLightDM::Greeter::MessageType)),
+                this, SLOT(handleMessage(pam_handle *, QString, QLightDM::Greeter::MessageType)));
         // This next connect is how we pass ResponseFutures between threads
-        connect(this, SIGNAL(showPrompt(QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)),
-                this, SLOT(handlePrompt(QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)));
+        connect(this, SIGNAL(showPrompt(pam_handle *, QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)),
+                this, SLOT(handlePrompt(pam_handle *, QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)));
     }
 
     void start(QString username)
@@ -64,14 +70,19 @@
             pam_end(handle, PAM_CONV_ERR);
         }
 
+        AppData *appData = new AppData();
+        appData->impl = this;
+
         // Now actually start a new conversation with PAM
         pam_conv conversation;
         conversation.conv = converseWithPam;
-        conversation.appdata_ptr = static_cast<void*>(this);
+        conversation.appdata_ptr = static_cast<void*>(appData);
 
         if (pam_start("lightdm", username.toUtf8(), &conversation, &pamHandle) == PAM_SUCCESS) {
+            appData->handle = pamHandle;
             futureWatcher.setFuture(QtConcurrent::run(authenticateWithPam, pamHandle));
         } else {
+            delete appData;
             greeterPrivate->authenticated = false;
             Q_EMIT greeter->showMessage("Internal error: could not start PAM authentication", QLightDM::Greeter::MessageTypeError);
             Q_EMIT greeter->authenticationComplete();
@@ -103,7 +114,9 @@
         if (!tmp_response)
             return PAM_CONV_ERR;
 
-        GreeterImpl* impl = static_cast<GreeterImpl*>(appdata_ptr);
+        AppData *appData = static_cast<AppData*>(appdata_ptr);
+        GreeterImpl *impl = appData->impl;
+        pam_handle *handle = appData->handle;
 
         int count;
         QVector<ResponseFuture> responses;
@@ -117,7 +130,7 @@
                 QString message(msg[count]->msg);
                 responses.append(ResponseFuture());
                 responses.last().reportStarted();
-                Q_EMIT impl->showPrompt(message, Greeter::PromptTypeQuestion, responses.last());
+                Q_EMIT impl->showPrompt(handle, message, Greeter::PromptTypeQuestion, responses.last());
                 break;
             }
             case PAM_PROMPT_ECHO_OFF:
@@ -125,19 +138,19 @@
                 QString message(msg[count]->msg);
                 responses.append(ResponseFuture());
                 responses.last().reportStarted();
-                Q_EMIT impl->showPrompt(message, Greeter::PromptTypeSecret, responses.last());
+                Q_EMIT impl->showPrompt(handle, message, Greeter::PromptTypeSecret, responses.last());
                 break;
             }
             case PAM_TEXT_INFO:
             {
                 QString message(msg[count]->msg);
-                Q_EMIT impl->showMessage(message, Greeter::MessageTypeInfo);
+                Q_EMIT impl->showMessage(handle, message, Greeter::MessageTypeInfo);
                 break;
             }
             default:
             {
                 QString message(msg[count]->msg);
-                Q_EMIT impl->showMessage(message, Greeter::MessageTypeError);
+                Q_EMIT impl->showMessage(handle, message, Greeter::MessageTypeError);
                 break;
             }
             }
@@ -159,6 +172,8 @@
             }
         }
 
+        delete appData;
+
         if (raise_error)
         {
             for (int i = 0; i < count; ++i)
@@ -186,8 +201,8 @@
     }
 
 Q_SIGNALS:
-    void showMessage(QString text, QLightDM::Greeter::MessageType type);
-    void showPrompt(QString text, QLightDM::Greeter::PromptType type, QLightDM::GreeterImpl::ResponseFuture response);
+    void showMessage(pam_handle *handle, QString text, QLightDM::Greeter::MessageType type);
+    void showPrompt(pam_handle *handle, QString text, QLightDM::Greeter::PromptType type, QLightDM::GreeterImpl::ResponseFuture response);
 
 private Q_SLOTS:
     void finishPam()
@@ -205,8 +220,22 @@
         Q_EMIT greeter->authenticationComplete();
     }
 
-    void handlePrompt(QString text, QLightDM::Greeter::PromptType type, QLightDM::GreeterImpl::ResponseFuture future)
-    {
+    void handleMessage(pam_handle *handle, QString text, QLightDM::Greeter::MessageType type)
+    {
+        if (handle != pamHandle)
+            return;
+
+        Q_EMIT greeter->showMessage(text, type);
+    }
+
+    void handlePrompt(pam_handle *handle, QString text, QLightDM::Greeter::PromptType type, QLightDM::GreeterImpl::ResponseFuture future)
+    {
+        if (handle != pamHandle) {
+            future.reportResult(QString());
+            future.reportFinished();
+            return;
+        }
+
         futures.enqueue(future);
         Q_EMIT greeter->showPrompt(text, type);
     }