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

Proposed by Michael Terry on 2014-08-19
Status: Merged
Approved by: Michael Terry on 2014-08-21
Approved revision: 1180
Merged at revision: 1192
Proposed branch: lp:~mterry/unity8/dismiss-old-pam-prompts
Merge into: lp:unity8
Diff against target: 153 lines (+43/-15)
2 files modified
qml/Greeter/GreeterContent.qml (+0/-1)
tests/mocks/LightDM/demo/GreeterPrivate.cpp (+43/-14)
To merge this branch: bzr merge lp:~mterry/unity8/dismiss-old-pam-prompts
Reviewer Review Type Date Requested Status
Michał Sawicz 2014-08-19 Approve on 2014-08-21
PS Jenkins bot (community) continuous-integration Approve on 2014-08-19
Stephen M. Webb (community) 2014-08-19 Approve on 2014-08-19
Review via email: mp+231363@code.launchpad.net

Commit message

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.)

Description of the change

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

To post a comment you must log in.
Michael Terry (mterry) wrote :

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) wrote :

Works for me on the desktop.

review: Approve
Michał Sawicz (saviq) wrote :

 * 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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'qml/Greeter/GreeterContent.qml'
2--- qml/Greeter/GreeterContent.qml 2014-06-11 15:36:51 +0000
3+++ qml/Greeter/GreeterContent.qml 2014-08-19 13:44:40 +0000
4@@ -87,7 +87,6 @@
5
6 onLoaded: {
7 item.currentIndex = greeterContentLoader.currentIndex;
8- item.resetAuthentication();
9 }
10
11 Binding {
12
13=== modified file 'tests/mocks/LightDM/demo/GreeterPrivate.cpp'
14--- tests/mocks/LightDM/demo/GreeterPrivate.cpp 2014-07-15 16:38:02 +0000
15+++ tests/mocks/LightDM/demo/GreeterPrivate.cpp 2014-08-19 13:44:40 +0000
16@@ -33,6 +33,12 @@
17 {
18 Q_OBJECT
19
20+ struct AppData
21+ {
22+ GreeterImpl *impl;
23+ pam_handle *handle;
24+ };
25+
26 typedef QFutureInterface<QString> ResponseFuture;
27
28 public:
29@@ -46,11 +52,11 @@
30
31 connect(&futureWatcher, SIGNAL(finished()),
32 this, SLOT(finishPam()));
33- connect(this, SIGNAL(showMessage(QString, QLightDM::Greeter::MessageType)),
34- greeter, SIGNAL(showMessage(QString, QLightDM::Greeter::MessageType)));
35+ connect(this, SIGNAL(showMessage(pam_handle *, QString, QLightDM::Greeter::MessageType)),
36+ this, SLOT(handleMessage(pam_handle *, QString, QLightDM::Greeter::MessageType)));
37 // This next connect is how we pass ResponseFutures between threads
38- connect(this, SIGNAL(showPrompt(QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)),
39- this, SLOT(handlePrompt(QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)));
40+ connect(this, SIGNAL(showPrompt(pam_handle *, QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)),
41+ this, SLOT(handlePrompt(pam_handle *, QString, QLightDM::Greeter::PromptType, QLightDM::GreeterImpl::ResponseFuture)));
42 }
43
44 void start(QString username)
45@@ -64,14 +70,19 @@
46 pam_end(handle, PAM_CONV_ERR);
47 }
48
49+ AppData *appData = new AppData();
50+ appData->impl = this;
51+
52 // Now actually start a new conversation with PAM
53 pam_conv conversation;
54 conversation.conv = converseWithPam;
55- conversation.appdata_ptr = static_cast<void*>(this);
56+ conversation.appdata_ptr = static_cast<void*>(appData);
57
58 if (pam_start("lightdm", username.toUtf8(), &conversation, &pamHandle) == PAM_SUCCESS) {
59+ appData->handle = pamHandle;
60 futureWatcher.setFuture(QtConcurrent::run(authenticateWithPam, pamHandle));
61 } else {
62+ delete appData;
63 greeterPrivate->authenticated = false;
64 Q_EMIT greeter->showMessage("Internal error: could not start PAM authentication", QLightDM::Greeter::MessageTypeError);
65 Q_EMIT greeter->authenticationComplete();
66@@ -103,7 +114,9 @@
67 if (!tmp_response)
68 return PAM_CONV_ERR;
69
70- GreeterImpl* impl = static_cast<GreeterImpl*>(appdata_ptr);
71+ AppData *appData = static_cast<AppData*>(appdata_ptr);
72+ GreeterImpl *impl = appData->impl;
73+ pam_handle *handle = appData->handle;
74
75 int count;
76 QVector<ResponseFuture> responses;
77@@ -117,7 +130,7 @@
78 QString message(msg[count]->msg);
79 responses.append(ResponseFuture());
80 responses.last().reportStarted();
81- Q_EMIT impl->showPrompt(message, Greeter::PromptTypeQuestion, responses.last());
82+ Q_EMIT impl->showPrompt(handle, message, Greeter::PromptTypeQuestion, responses.last());
83 break;
84 }
85 case PAM_PROMPT_ECHO_OFF:
86@@ -125,19 +138,19 @@
87 QString message(msg[count]->msg);
88 responses.append(ResponseFuture());
89 responses.last().reportStarted();
90- Q_EMIT impl->showPrompt(message, Greeter::PromptTypeSecret, responses.last());
91+ Q_EMIT impl->showPrompt(handle, message, Greeter::PromptTypeSecret, responses.last());
92 break;
93 }
94 case PAM_TEXT_INFO:
95 {
96 QString message(msg[count]->msg);
97- Q_EMIT impl->showMessage(message, Greeter::MessageTypeInfo);
98+ Q_EMIT impl->showMessage(handle, message, Greeter::MessageTypeInfo);
99 break;
100 }
101 default:
102 {
103 QString message(msg[count]->msg);
104- Q_EMIT impl->showMessage(message, Greeter::MessageTypeError);
105+ Q_EMIT impl->showMessage(handle, message, Greeter::MessageTypeError);
106 break;
107 }
108 }
109@@ -159,6 +172,8 @@
110 }
111 }
112
113+ delete appData;
114+
115 if (raise_error)
116 {
117 for (int i = 0; i < count; ++i)
118@@ -186,8 +201,8 @@
119 }
120
121 Q_SIGNALS:
122- void showMessage(QString text, QLightDM::Greeter::MessageType type);
123- void showPrompt(QString text, QLightDM::Greeter::PromptType type, QLightDM::GreeterImpl::ResponseFuture response);
124+ void showMessage(pam_handle *handle, QString text, QLightDM::Greeter::MessageType type);
125+ void showPrompt(pam_handle *handle, QString text, QLightDM::Greeter::PromptType type, QLightDM::GreeterImpl::ResponseFuture response);
126
127 private Q_SLOTS:
128 void finishPam()
129@@ -205,8 +220,22 @@
130 Q_EMIT greeter->authenticationComplete();
131 }
132
133- void handlePrompt(QString text, QLightDM::Greeter::PromptType type, QLightDM::GreeterImpl::ResponseFuture future)
134- {
135+ void handleMessage(pam_handle *handle, QString text, QLightDM::Greeter::MessageType type)
136+ {
137+ if (handle != pamHandle)
138+ return;
139+
140+ Q_EMIT greeter->showMessage(text, type);
141+ }
142+
143+ void handlePrompt(pam_handle *handle, QString text, QLightDM::Greeter::PromptType type, QLightDM::GreeterImpl::ResponseFuture future)
144+ {
145+ if (handle != pamHandle) {
146+ future.reportResult(QString());
147+ future.reportFinished();
148+ return;
149+ }
150+
151 futures.enqueue(future);
152 Q_EMIT greeter->showPrompt(text, type);
153 }

Subscribers

People subscribed via source and target branches