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

[Brian Cameron <Brian Cameron Sun COM>]

Ludwig:

> Brian Cameron wrote:
> > Can you resend the patch updated based on our discussion so far?  I'd
> > like to get this upstream but would like to see a patch that takes
> > into account our discussion so far.
>
> I've split up the patch into smaller chunks.

Thanks.  This makes it much easier to review.   I accepted most of your
changes into SVN head...aside from the one below I comment on.

> ------------------------------------------------------------------------
>
> - don't make the start again button insensitive as the prompt for
>   the username is not necessarily the first.

Why change the calls to set_sensitive to always be TRUE, why not just
rip out all calls to set the sensitivity for this button?

Also, might there be a better solution.  We want the "Start Again"
button to be active at all times except when the GUI is prompting for
the first item.  Perhaps trying to set button sensitivity in the
current GDM_PROMPT, GDM_NOECHO locations isn't the right place.  So I
ask if the right fix is to rip out setting sensitivity or to move it so
it is setting it more intelligently?

I think this change also needs to be made to gdmgreeter.

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

I assume this is no longer needed because we are now managing this
in verify-pam.c?  Is this correct?  But what about if GDM is using
verify-crypt.c or verify-shadow.c, don't we also need some similar
sort of hack to make this work the same for crypt/shadow?

I made the similar change to gdmgreeter, so hopefully that will work
properly also now.

> - allow the message boxes to expand

Accepted.

I don't like just surrounding code with #if 0 and #endif.  Just
remove the lines.  I made the change in this way.

> - don't preset message with "please enter your username" as the
>   first prompt might be a different one.

Accepted.  Need to check if gdmgreeter has a similar issue.

> 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);
>  	}
> @@ -2686,7 +2688,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);
> @@ -2756,14 +2760,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,
>
>
>
> ------------------------------------------------------------------------
>
> - display lastlog information after succesful authentication

accepted.

> ------------------------------------------------------------------------
>
> - cancel all conversations in a pam_authenticate run if a user was
>   selected in the face browser.

accepted.

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

accepted.

> ------------------------------------------------------------------------
>
> - remove the tmp_PAM_USER hack
> - redo auth with same username only until LOGIN_RETRIES is reached

accepted.

Brian