Merge lp:~christian-w/lightdm/pam-multi-prompts into lp:lightdm

[Christian Seiler]

Add signal indicating the end of the PAM conversation round

    Some PAM modules generate multiple prompts in one PAM conversation
    round. This patch adds a signal show-round-complete (Glib bindings) /
    showRoundComplete (Qt bindings) to the API that is emitted after all
    show_prompt / show_message signals of the current PAM conversation
    round. This signal should make it easier for people developing greeters
    to react to the case of multiple prompts.

    (An example PAM module that uses multiple prompts is pam_krb5 when the
    needchange attribute is set on a principal.)

[Robert Ancell (robert-ancell)]

Thanks for this Christian!

Can you remove this line - consumers of this API should require LightDM 1.9.7 if they need this API.
+#define LIGHTDM_HAVE_SHOW_ROUND_COMPLETE 1

This change is good, but please move it to a separate merge proposal:
+Greeter::PromptType GreeterPrivate::promptTypeFromGlib(LightDMPromptType type)

[Robert Ancell (robert-ancell)]

review: Needs Fixing

[Christian Seiler (christian-w)]

> Thanks for this Christian!
>
> Can you remove this line - consumers of this API should require LightDM 1.9.7 if they need this API.
> +#define LIGHTDM_HAVE_SHOW_ROUND_COMPLETE 1
>
> This change is good, but please move it to a separate merge proposal:
> +Greeter::PromptType GreeterPrivate::promptTypeFromGlib(LightDMPromptType type)

I pondered the whole issue for a while and I now actually think that
this new signal is not actually needed, even if you ask for stuff
sequentially.

Let me make the following assumption:

 * There is only a single thread processing the signals from greeter,
   at least when it comes to the UI and the response to those signals.
   (Logging/Debugging them additionally doesn't matter.)

   For glib this is the case anyway AFAIK, since its signals are
   always callbacks. Qt supports signals crossing thread boundaries
   (in which case they are not callbacks, but dispatched by event
   loops), but even if the signals cross thread boundaries, you'd
   have to have two different threads receiving different signals
   for this assumption to break down, and that already breaks the
   assumption behind the whole point of the patch in the first place,
   i.e. that there is some kind of order of the signals.

So, as long as you have any greeter that fulfills the above assumption
(and I think all current greeters do and I can't think of any sane use
case for any greeter not fulfilling that), the following logic should be
able to cope with multiple signals for sequential logic:

common variables: pending_prompts, prompt_active

(E1) on prompt:
  1. append info to pending_prompts
  2. if not prompt_active:
       2a. call processing function

(E2) on "user presses enter":
  1. prompt_active = false
  2. respond to prompt
  3. if pending_prompts:
       3a. call processing function

processing function:
  1. show first prompt from pending_prompts
  2. prompt_active = true

If you now have two prompts at once, then you have two (E1) events.
Ideally, you have something like (E1) (E1) (E2) (E2). But also in the
case of (E1) (E2) (E1) (E2) this shouldn't really be a problem. (E1)
(E2) (E2) (E1) can't happen if enter presses with no active prompt are
suppressed properly.

Since you said that at some point in the future you might want to rework
the whole API anyway, and I now realize that for fixing the other
greeters I actually don't need this change in liblightdm-{gobject,qt}, I
don't think adding some stuff to the current API is all that helpful in
the long run, especially if it is not really needed. I therefore propose:

1. You reject this merge request (I'm new to launchpad, can I actually
take it back?)

2. I write a trivial merge request for the Qt bindings that fix the
prompt types.

3. I go on to fix the Gtk and KDE greeters in this regard, but so that
they work with the current API, which also means that they don't need
any compatibility code for this (makes the code much easier to read).

Do you agree?

Regards,
Christian

[Robert Ancell (robert-ancell)]

Yes, sounds good.

I was asking around for ideas on the best way to remove merge proposals.

- You can abandon your branch by going to https://code.launchpad.net/~christian-w/lightdm/pam-multi-prompts and setting the status to "Abandoned". We *think* that will automatically obsolete the MP (let's try).
- Or, you can select "Delete proposal to merge" from the top right of https://code.launchpad.net/~christian-w/lightdm/pam-multi-prompts/+merge/205024. But that erases it from history which is a little annoying because it's nice to see the reasoning at a later date if necessary.
- Or, I can set the status of the merge proposal to Rejected, but the wording of that always seems a bit harsh to me :)

[Christian Seiler (christian-w)]

> - You can abandon your branch by going to
> https://code.launchpad.net/~christian-w/lightdm/pam-multi-prompts
> and setting the status to "Abandoned". We *think* that will
> automatically obsolete the MP (let's try).

Just tried that, after doing that there was the additional message

"The branch target has been changed to Light Display Manager (lightdm)"

which didn't make much sense to me because I thought that was already
the target?

Regardless, it's now set to Abandoned, but the merge proposal is still
active, AFAICS...

> - Or, I can set the status of the merge proposal to Rejected, but
> the wording of that always seems a bit harsh to me :)

Nah, just do that if you prefer to keep the history.

Regards,
Christian

PS: FYI, for the Gtk greeter:
https://code.launchpad.net/~christian-w/lightdm-gtk-greeter/pam-multiple-prompts/+merge/205254

Unmerged revisions

1881. By Christian Seiler on 2014-02-05

Add signal indicating the end of the PAM conversation round

Some PAM modules generate multiple prompts in one PAM conversation
round. This patch adds a signal show-round-complete (Glib bindings) /
showRoundComplete (Qt bindings) to the API that is emitted after all
show_prompt / show_message signals of the current PAM conversation
round. This signal should make it easier for people developing greeters
to react to the case of multiple prompts.

(An example PAM module that uses multiple prompts is pam_krb5 when the
needchange attribute is set on a principal.)

=== modified file 'liblightdm-gobject/greeter.c'
--- liblightdm-gobject/greeter.c	2013-07-23 02:24:45 +0000
+++ liblightdm-gobject/greeter.c	2014-02-05 18:09:21 +0000
@@ -39,6 +39,7 @@
     SHOW_MESSAGE,
     AUTHENTICATION_COMPLETE,
     AUTOLOGIN_TIMER_EXPIRED,
+    SHOW_ROUND_COMPLETE,
     LAST_SIGNAL
 };
 static guint signals[LAST_SIGNAL] = { 0 };
@@ -341,6 +342,8 @@
 
         g_free (text);
     }
+
+    g_signal_emit (G_OBJECT (greeter), signals[SHOW_ROUND_COMPLETE], 0);
 }
 
 static void
@@ -1394,4 +1397,24 @@
                       NULL, NULL,
                       NULL,
                       G_TYPE_NONE, 0);
+
+    /**
+     * LightDMGreeter::show-round-complete:
+     * @greeter: A #LightDMGreeter
+     *
+     * The ::show-round-complete signal gets emitted when all the
+     * ::show-prompt and ::show-message signals for the current
+     * PAM conversation round have been emitted. With this signal,
+     * clients can collect all messages and prompts first before
+     * reacting to it.
+     **/
+    signals[SHOW_ROUND_COMPLETE] =
+        g_signal_new ("show-round-complete",
+                      G_TYPE_FROM_CLASS (klass),
+                      G_SIGNAL_RUN_LAST,
+                      G_STRUCT_OFFSET (LightDMGreeterClass, show_round_complete),
+                      NULL, NULL,
+                      NULL,
+                      G_TYPE_NONE, 0);
+
 }

=== modified file 'liblightdm-gobject/liblightdm-gobject-1.vapi'
--- liblightdm-gobject/liblightdm-gobject-1.vapi	2012-10-01 01:58:44 +0000
+++ liblightdm-gobject/liblightdm-gobject-1.vapi	2014-02-05 18:09:21 +0000
@@ -23,6 +23,7 @@
         public signal void show_prompt (string text, PromptType type);
         public signal void authentication_complete ();
         public signal void autologin_timer_expired ();
+        public signal void show_round_complete ();
 
         public bool connect_sync () throws GLib.Error;
         public unowned string get_hint (string name);

=== modified file 'liblightdm-gobject/lightdm/greeter.h'
--- liblightdm-gobject/lightdm/greeter.h	2013-07-23 02:24:45 +0000
+++ liblightdm-gobject/lightdm/greeter.h	2014-02-05 18:09:21 +0000
@@ -12,6 +12,7 @@
 #define LIGHTDM_GREETER_H_
 
 #include <glib-object.h>
+#include <glib/gslist.h>
 
 G_BEGIN_DECLS
 
@@ -22,6 +23,8 @@
 #define LIGHTDM_IS_GREETER_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), LIGHTDM_TYPE_GREETER))
 #define LIGHTDM_GREETER_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), LIGHTDM_TYPE_GREETER, LightDMGreeterClass))
 
+#define LIGHTDM_HAVE_SHOW_ROUND_COMPLETE  1
+
 /**
  * LightDMPromptType:
  * @LIGHTDM_PROMPT_TYPE_QUESTION: Prompt is a question.  The information can be shown as it is entered.
@@ -57,6 +60,7 @@
     void (*show_prompt)(LightDMGreeter *greeter, const gchar *text, LightDMPromptType type);
     void (*authentication_complete)(LightDMGreeter *greeter);
     void (*autologin_timer_expired)(LightDMGreeter *greeter);
+    void (*show_round_complete)(LightDMGreeter *greeter);
 
     /* Reserved */
     void (*reserved1) (void);
@@ -64,7 +68,6 @@
     void (*reserved3) (void);
     void (*reserved4) (void);
     void (*reserved5) (void);
-    void (*reserved6) (void);
 } LightDMGreeterClass;
 
 GType lightdm_greeter_get_type (void);

=== modified file 'liblightdm-qt/QLightDM/greeter.h'
--- liblightdm-qt/QLightDM/greeter.h	2013-04-16 16:37:08 +0000
+++ liblightdm-qt/QLightDM/greeter.h	2014-02-05 18:09:21 +0000
@@ -14,7 +14,9 @@
 
 #include <QtCore/QObject>
 #include <QtCore/QVariant>
+#include <QtCore/QList>
 
+#define QLIGHTDM_HAVE_SHOW_ROUND_COMPLETE
 
 namespace QLightDM
 {
@@ -86,12 +88,14 @@
     void showPrompt(QString text, QLightDM::Greeter::PromptType type);
     void authenticationComplete();
     void autologinTimerExpired();
+    void showRoundComplete();
 
 private:
     GreeterPrivate *d_ptr;
     Q_DECLARE_PRIVATE(Greeter)
 
 };
+
 }
 
 #endif // QLIGHTDM_GREETER_H

=== modified file 'liblightdm-qt/greeter.cpp'
--- liblightdm-qt/greeter.cpp	2013-02-06 14:08:25 +0000
+++ liblightdm-qt/greeter.cpp	2014-02-05 18:09:21 +0000
@@ -33,7 +33,10 @@
     static void cb_showMessage(LightDMGreeter *greeter, const gchar *text, LightDMMessageType type, gpointer data);
     static void cb_authenticationComplete(LightDMGreeter *greeter, gpointer data);
     static void cb_autoLoginExpired(LightDMGreeter *greeter, gpointer data);
-    
+    static void cb_showRoundComplete(LightDMGreeter *greeter, gpointer data);
+
+    static Greeter::PromptType promptTypeFromGlib(LightDMPromptType type);
+    static Greeter::MessageType messageTypeFromGlib(LightDMMessageType type);
 private:
     Q_DECLARE_PUBLIC(Greeter)
 };
@@ -50,6 +53,7 @@
     g_signal_connect (ldmGreeter, "show-message", G_CALLBACK (cb_showMessage), this);
     g_signal_connect (ldmGreeter, "authentication-complete", G_CALLBACK (cb_authenticationComplete), this);
     g_signal_connect (ldmGreeter, "autologin-timer-expired", G_CALLBACK (cb_autoLoginExpired), this);
+    g_signal_connect (ldmGreeter, "show-round-complete", G_CALLBACK (cb_showRoundComplete), this);
 }
 
 void GreeterPrivate::cb_showPrompt(LightDMGreeter *greeter, const gchar *text, LightDMPromptType type, gpointer data)
@@ -59,9 +63,7 @@
     GreeterPrivate *that = static_cast<GreeterPrivate*>(data);
     QString message = QString::fromUtf8(text);
     
-    //FIXME prompt type
-
-    Q_EMIT that->q_func()->showPrompt(message, Greeter::PromptTypeSecret);
+    Q_EMIT that->q_func()->showPrompt(message, promptTypeFromGlib(type));
 }
 
 void GreeterPrivate::cb_showMessage(LightDMGreeter *greeter, const gchar *text, LightDMMessageType type, gpointer data)
@@ -71,9 +73,7 @@
     GreeterPrivate *that = static_cast<GreeterPrivate*>(data);
     QString message = QString::fromUtf8(text);
 
-    //FIXME prompt type
-
-    Q_EMIT that->q_func()->showMessage(message, Greeter::MessageTypeInfo);
+    Q_EMIT that->q_func()->showMessage(message, messageTypeFromGlib(type));
 }
 
 void GreeterPrivate::cb_authenticationComplete(LightDMGreeter *greeter, gpointer data)
@@ -90,6 +90,29 @@
     Q_EMIT that->q_func()->autologinTimerExpired();
 }
 
+void GreeterPrivate::cb_showRoundComplete(LightDMGreeter *greeter, gpointer data)
+{
+    Q_UNUSED(greeter);
+    GreeterPrivate *that = static_cast<GreeterPrivate*>(data);
+    Q_EMIT that->q_func()->showRoundComplete();
+}
+
+Greeter::PromptType GreeterPrivate::promptTypeFromGlib(LightDMPromptType type)
+{
+    if (type == LIGHTDM_PROMPT_TYPE_QUESTION)
+        return Greeter::PromptTypeQuestion;
+    else
+        return Greeter::PromptTypeSecret;
+}
+
+Greeter::MessageType GreeterPrivate::messageTypeFromGlib(LightDMMessageType type)
+{
+    if (type == LIGHTDM_MESSAGE_TYPE_INFO)
+        return Greeter::MessageTypeInfo;
+    else
+        return Greeter::MessageTypeError;
+}
+
 Greeter::Greeter(QObject *parent) :
     QObject(parent),
     d_ptr(new GreeterPrivate(this))