[gdm-list] [PATCH] some pam related improvements

[Ludwig Nussel <ludwig nussel suse de>]

Hi,

I've been playing with gdm and my pam test module a bit yesterday.
Here are some changes I'd like to suggest:

gdmlogin:
- don't make the start again button insensitive as the prompt for
  the username is not necessarily the first.
- don't tell backend that a user was selected in the face browser if
  the backend itself caused that to avoid aborting an ongoing
  authentication.
- allow the message boxes to expand
- don't preset message with "please enter your username" as the
  first prompt might be a different one.

verify-pam:
- remove the stange tmp_PAM_USER hack
- cancel all conversations in a pam_authenticate run if a user was
  selected in the face browser.
- don't try to be smart about the username prompt. If pam asks for a
  username then also ask the user about it. The GUI could be made to
  preset the text entry with the username instead.
- remove the lastlogin hack. pam_lastlog can do that *after*
  authentication instead.

cu
Ludwig

Index: gdm2/gui/gdmlogin.c
===================================================================
--- gdm2.orig/gui/gdmlogin.c
+++ gdm2/gui/gdmlogin.c
@@ -1558,7 +1558,8 @@ process_operation (guchar       op_code,
 					gdm_config_get_bool   (GDM_KEY_SOUND_ON_LOGIN));
 		gtk_label_set_text_with_mnemonic (GTK_LABEL (label), _("_Username:"));
 		greeter_probably_login_prompt = TRUE;
-		gtk_widget_set_sensitive (start_again_button, FALSE);
+		/* prompt for username is not necessarily the first */
+		gtk_widget_set_sensitive (start_again_button, TRUE);
 	} else {
 		gtk_widget_set_sensitive (start_again_button, TRUE);
 		if (tmp != NULL)
@@ -1584,17 +1585,18 @@ process_operation (guchar       op_code,
 
 	login_window_resize (FALSE /* force */);
 
+#if 0 /* sends interruption to backend which would cause it to redo auth */
         if (greeter_probably_login_prompt == TRUE && selected_user != NULL)
 		face_browser_select_user (selected_user);
+#endif
 	break;
 
     case GDM_NOECHO:
 	tmp = ve_locale_to_utf8 (args);
+	gtk_widget_set_sensitive (start_again_button, TRUE);
 	if (tmp != NULL && strcmp (tmp, _("Password:")) == 0) {
-		gtk_widget_set_sensitive (start_again_button, TRUE);
 		gtk_label_set_text_with_mnemonic (GTK_LABEL (label), _("_Password:"));
 	} else {
-		gtk_widget_set_sensitive (start_again_button, FALSE);
 		if (tmp != NULL)
 			gtk_label_set_text (GTK_LABEL (label), tmp);
 	}
@@ -2687,7 +2689,9 @@ gdm_login_gui_init (void)
     /* Put in error box here */
     err_box = gtk_label_new (NULL);
     gtk_widget_set_name (err_box, "Error box");
+#if 0 /* widget doesn't expand anymore after this call */
     gtk_widget_set_size_request (err_box, -1, 40);
+#endif
     g_signal_connect (G_OBJECT (err_box), "destroy",
 		      G_CALLBACK (gtk_widget_destroyed),
 		      &err_box);
@@ -2757,14 +2761,16 @@ gdm_login_gui_init (void)
 		      (GtkAttachOptions) (GTK_EXPAND | GTK_FILL),
 		      (GtkAttachOptions) (GTK_FILL), 0, 10);
 
-    msg = gtk_label_new (_("Please enter your username"));
+    msg = gtk_label_new (NULL);
     gtk_widget_set_name (msg, "Message");
     gtk_label_set_line_wrap (GTK_LABEL (msg), TRUE);
     gtk_label_set_justify (GTK_LABEL (msg), GTK_JUSTIFY_LEFT);
     gtk_table_attach (GTK_TABLE (stack), msg, 0, 1, 6, 7,
 		      (GtkAttachOptions) (GTK_EXPAND | GTK_FILL),
 		      (GtkAttachOptions) (GTK_FILL), 0, 10);
+#if 0 /* widget doesn't expand anymore after this call */
     gtk_widget_set_size_request (msg, -1, 30);
+#endif
 
     gtk_widget_ref (msg);
     g_object_set_data_full (G_OBJECT (login), "msg", msg,
Index: gdm2/daemon/verify-pam.c
===================================================================
--- gdm2.orig/daemon/verify-pam.c
+++ gdm2/daemon/verify-pam.c
@@ -52,9 +52,6 @@ static pam_handle_t *pamh = NULL;
 
 static GdmDisplay *cur_gdm_disp = NULL;
 
-/* this is a hack */
-static char *tmp_PAM_USER = NULL;
-
 /* this is another hack */
 static gboolean did_we_ask_for_password = FALSE;
 
@@ -489,7 +486,9 @@ gdm_verify_pam_conv (int num_msg, struct
     /* Should never happen unless PAM is on crack and keeps asking questions
        after we told it to go away.  So tell it to go away again and
        maybe it will listen */
-    if ( ! gdm_slave_action_pending ())
+    /* well, it actually happens if there are multiple pam modules
+     * with conversations */
+    if ( ! gdm_slave_action_pending () || selected_user)
         return PAM_CONV_ERR;
 
     reply = malloc (sizeof (struct pam_response) * num_msg);
@@ -502,13 +501,8 @@ gdm_verify_pam_conv (int num_msg, struct
     /* Here we set the login if it wasn't already set,
      * this is kind of anal, but this way we guarantee that
      * the greeter always is up to date on the login */
-    if (pam_get_item (pamh, PAM_USER, (void **)&p) == PAM_SUCCESS ||
-	tmp_PAM_USER != NULL) {
+    if (pam_get_item (pamh, PAM_USER, (void **)&p) == PAM_SUCCESS) {
 	    login = (const char *)p;
-
-	    /* FIXME: this is a HACK HACK HACK, for some reason needed */
-	    if (tmp_PAM_USER != NULL)
-		    login = tmp_PAM_USER;
 	    gdm_slave_greeter_ctl_no_ret (GDM_SETLOGIN, login);
     }
 
@@ -517,7 +511,6 @@ gdm_verify_pam_conv (int num_msg, struct
     openlog ("gdm", LOG_PID, LOG_DAEMON);
     
     for (replies = 0; replies < num_msg; replies++) {
-	gboolean islogin = FALSE;
 	const char *m = (*msg)[replies].msg;
 	m = perhaps_translate_message (m);
 	
@@ -526,13 +519,7 @@ gdm_verify_pam_conv (int num_msg, struct
 	/* PAM requested textual input with echo on */
 	case PAM_PROMPT_ECHO_ON:
  	    if (strcmp (m, _("Username:")) == 0) {
-		    if ( ! ve_string_empty (selected_user)) {
-			    /* Sometimes we are just completely on crack,
-			       and pam asks for the username even if we
-			       already gave it.  PAM is on better crack,
-			       then I can afford. */
-			    s = g_strdup (selected_user);
-		    } else {
+		    if ( ve_string_empty (selected_user)) {
 			    /* this is an evil hack, but really there is no way we'll
 			    know this is a username prompt.  However we SHOULD NOT
 			    rely on this working.  The pam modules can set their
@@ -542,7 +529,6 @@ gdm_verify_pam_conv (int num_msg, struct
 			    s = gdm_slave_greeter_ctl (GDM_PROMPT, m);
 			    /* this will clear the message */
 			    gdm_slave_greeter_ctl_no_ret (GDM_MSG, "");
-			    islogin = TRUE;
 		    }
 	    } else {
 		    s = gdm_slave_greeter_ctl (GDM_PROMPT, m);
@@ -557,29 +543,6 @@ gdm_verify_pam_conv (int num_msg, struct
 		    return PAM_CONV_ERR;
 	    }
 
-	    if (islogin) {
-		    /* note that we should NOT rely on this for anything
-		       that is REALLY needed.  The reason this is here
-		       is for the face browser to work correctly.  The
-		       best way would really be to ask all questions at
-		       once, but that would 1) need some protocol rethinking,
-		       2) the gdmgreeter program is really not equipped
-		       for this, would need theme format change */
-		    /* Second problem is that pam will not actually set
-		       PAM_USER itself after it asks for it.  This is
-		       pretty evil, evil, evil, evil, but somewhat necessary
-		       then. */
-		    /* FIXME: this is a HACK HACK HACK */
-		    g_free (tmp_PAM_USER);
-		    tmp_PAM_USER = g_strdup (s);
-
-		    if (gdm_get_value_bool (GDM_KEY_DISPLAY_LAST_LOGIN)) {
-			    char *info = gdm_get_last_info (s);
-			    gdm_slave_greeter_ctl_no_ret (GDM_ERRBOX, info);
-			    g_free (info);
-		    }
-	    }
-
 	    reply[replies].resp_retcode = PAM_SUCCESS;
 	    reply[replies].resp = strdup (ve_sure_string (s));
 	    g_free (s);
@@ -874,10 +837,6 @@ verify_user_again:
 
 authenticate_again:
 
-    /* hack */
-    g_free (tmp_PAM_USER);
-    tmp_PAM_USER = g_strdup (login);
-
     /*
      * Initialize a PAM session for the user...
      * Get value per-display so different displays can use different
@@ -894,6 +853,9 @@ authenticate_again:
     }
     g_free (pam_stack);
 
+    g_free (login);
+    login = NULL; // have to unset it otherwise there is no chance to ever enter a different user
+
     pam_set_item (pamh, PAM_USER_PROMPT, _("Username:"));
 
 #if 0
@@ -940,7 +902,7 @@ authenticate_again:
 
 		    /* FIXME: what about errors */
 		    /* really this has been a sucess, not a failure */
-		    pam_end (tmp_pamh, PAM_SUCCESS);
+		    pam_end (tmp_pamh, pamerr);
 
 		    gdm_slave_greeter_ctl_no_ret (GDM_SETLOGIN, login);
 
@@ -1119,9 +1081,6 @@ authenticate_again:
 
     cur_gdm_disp = NULL;
 
-    g_free (tmp_PAM_USER);
-    tmp_PAM_USER = NULL;
-
 #ifdef  HAVE_LOGINDEVPERM
     if (d->attached)
 	(void) di_devperm_login ("/dev/console", pwent->pw_uid,
@@ -1206,9 +1165,6 @@ authenticate_again:
 
     g_free (login);
 
-    g_free (tmp_PAM_USER);
-    tmp_PAM_USER = NULL;
-    
     cur_gdm_disp = NULL;
 
     return NULL;
-- 
 (o_   Ludwig Nussel
 //\   SUSE LINUX Products GmbH, Development
 V_/_  http://www.suse.de/