GNOME Bugzilla – Bug 583856
gdm displays PAM_TEXT_INFO only very briefly
Last modified: 2011-05-19 15:55:09 UTC
In the old gdm (2.8), the PAM_TEXT_INFO will enforce gdm to popup a dialog to warn the users. But in the current gdm, this implement was not included. Two problems: 1. if there were two signals emitted nearly together, the first one will be covered too soon to be seen. 2. if the INFO was important but not prevent gdm from login, the gdm-greeter will quit immediately so the user have no chance to know what happens. Other information: http://bugzilla.novell.com/show_bug.cgi?id=439918
Created attachment 135364 [details] [review] popup warning dialog when the pam return PAM_TEXT_INFO The raise of the PAM_TEXT_INFO severity (to "problem") is just to inform gdm-simpler-greeter to popup a dialog, other normal informations will still be displayed at the auth-message-label. The codes of the popup dialog were pulled from gdm2.8.
Created attachment 141222 [details] [review] remove set_message as it may confuse the user I remove - set_message (GDM_GREETER_LOGIN_WINDOW (login_window), text); from the patch. I leaved this line in the previous patch is in order to display the TEXT_INFO message for a longer time. But the previous solution may confuse the user because the sequence of the PAM_TEXT_INFO/PAM_PROMPT_ECHO_OFF/PAM_PROMPT_ECHO_ON is important.
I see the same problem on Solaris. For example, if you expire a user's password by running "passwd -f". Then when you try to log in as the user, GDM does correctly ask the user to redefine their password. However, on Solaris if you type a new password less than 6 characters it will return the following PAM message "Password too short - must be at least 6 characters" or if you type a password without any numeric or special characters, then it will say "The password must contain at least 1 numeric or special character(s)." Then after the the previous message PAM returns a message that says "Authentication token manipulation error". Without this patch, the user only sees the useless "Authentication token manipulation error" and has no idea what was wrong with their new password. This patch does fix the problem, however it does seem ugly that all messages now appear in a little dialog. It might be nicer if the pop-up were only used if there are multiple messages to show, or if the login greeter window could be enhanced to just show multiple messages. Or perhaps the first message appear in the login dialog and the pop-up is only used if there are more messages to show.
The problem here is, 1) sometimes the PAM messages have the sequence relationship. So the users need a way to interact with greeter step by step. In this case, a dialog may be a good choice. 2) sometimes, the PAM messages have the child-parent relationship, just as you described. So the users need a way to display all the messages. In this case, the orignial auth-message-label is the good choice. But we need to provide something like 'append_message' other than 'set_message'. currently, auth-prompt-label and auth-message-label did not affect each other, how about append_message to auth-message-label when auth-prompt-label did not change. this is to solve problem 2) About 1), I don't know how to differentiate such PAM message yet.
The old GDM had two different text label areas. One for displaying PAM errors and the other for displaying PAM non-error messages. This is why the old GDM greeter themes, for example, supported both a "pam-message" and a "pam-error" text area. The old GDM also expanded the text areas so that they could show multiple messages since PAM can return up to 32 messages in a single message. Both of them used a scrolling region so the user could scroll them if there were too many to display in the defined region. I think the issue I reported in item #3 would be better resolved by returning to that older design. It worked well, and I don't remember any complaints about it.
This is really a serious issue. Not showing PAM messages is bad. Bumping up the priority/severity.
*** Bug 613031 has been marked as a duplicate of this bug. ***
Comment on attachment 141222 [details] [review] remove set_message as it may confuse the user I don't think we want to display a popup dialog.
Do you have an example of a PAM module that does this? So I can reproduce the problem? Thanks.
I don't have such module at hand and don't know how to modify a pam module. I added some dummy signals to gdm daemon, for example, add gdm_session_worker_report_info when the authentication was successful, so the bug can be reproduced.
As I mention in comment #3, the process of entering new passwords when a password is expired with "passwd -f" shows this problem on Solaris. Though to test this you would need to build with the patch and see if multiple pop-up dialogs with different messages show up at any stage in the PAM conversation. Otherwise you might not notice that some messages are being ignored (or displayed and overwritten so quickly you don't see them).
In order to try to address this we'll first need an example that works on Linux. Can anyone provide one? Thanks.
Jon, I wouldn't think it would be necessary to use a PAM module to test this. I would think that you could simulate the problem by hacking gdm_session_worker_process_pam_message so that when it falls into the PAM_TEXT_INFO case that it calls gdm_session_worker_report_info 2 or 3 times with various hardcoded messages. The problem is really with the GUI. The GUI should capture multiple messages and display them, perhaps in a scrolling text region like the old GDM. So, hacking the daemon to simulate multiple messages shouldn't be too hard to simulate. So, when the bug is fixed, I would think you could see all the hardcoded messages in the display area. I am happy to also verify any patch fixes the problem on OpenSolaris where I can see the problem easily with expired passwords as I mentioned in comment #3.
I'm not really that interested in hypothetical problems. If anyone has a real world example of where this is a problem that would be great. Thanks.
Here's a simple real-world example of where this is an issue. If the System Security Services Daemon is configured to handle cached network logins, then it emits a PAM_TEXT_INFO message after successful login to alert the user that the authentication happened offline. If SSSD is also configured with offline_credentials_expiration, then this message will also inform the user of how much time remains for them to operate offline before being required to connect and perform an online authentication. Since these messages in GDM appear only for a fraction of a second, it's difficult for a user to get this information that they need.
(In reply to comment #14) > I'm not really that interested in hypothetical problems. If anyone has a real > world example of where this is a problem that would be great. Thanks. A very simple example is pam_unix with password expiration policies in place. The message that the password is going to expire is not readable.
I have an in progress patch for this on the multistack branch: http://git.gnome.org/browse/gdm/commit/?h=multi-stack&id=5d9d29165e56af9ddd166961e868fae781686647 of course, it's not exactly the same on master. It's probably worth getting this fix rewritten for master before we merge multi-stack over.
Reopening as I think the requested information has been provided.
Created attachment 187237 [details] [review] Queue messages and show them all This patch should fix the bug in a correct way. I took inspiration from the change on the multistack branch, but also added a message queue to ensure that all messages are seen. This seems to fix all the facets of the problem that have been mentioned here.
Jos: Thanks for the patch. I did some testing with this, and it did not seem to work properly for me. It seems that the patch in comment #19 does show the multiple messages, but it seems to always restart the greeter when there are multiple messages to show. Does the patch in comment #19 assume that multiple messages only happens in error situations where the greeter should be restarted. There are situations on Solaris when there are multiple informational messages that should not restart the PAM conversation. This happens, for example, when resetting a password expired with "passwd -f". The GDM greeter should just display the multiple messages and allow the PAM conversation to continue. Also, it seems that you are showing the messages by showing them one at a time for a timeout period. Is this correct? This is perhaps better than not showing the message, but may not always give the user time to digest the message. The timeout seemed short to me. In general, I think it is better to let the user click a button to cycle through them or to show them in a scrolling area where the user can review them. Note that I tried to backport the patch in comment #19 to GDM 2.30.6. So perhaps the odd behaviors I was seeing was because the patch isn't intended to work in GDM older than 3. Not sure. I expected it to work better since the patch did apply fairly cleanly.
(In reply to comment #20) > Thanks for the patch. I did some testing with this, and it did not seem to > work properly for me. It seems that the patch in comment #19 does show the > multiple messages, but it seems to always restart the greeter when there are > multiple messages to show. Does the patch in comment #19 assume that multiple > messages only happens in error situations where the greeter should be > restarted. I tested it with password expiration messages - which do not imply a restart - and it worked properly for me. The greeter stays up for a few more seconds. That was with 3.0.0 though, I did not try 2.30.6. > Also, it seems that you are showing the messages by showing them one at a time > for a timeout period. Is this correct? This is perhaps better than not > showing the message, but may not always give the user time to digest the > message. The timeout seemed short to me. In general, I think it is better to > let the user click a button to cycle through them or to show them in a > scrolling area where the user can review them. Yes, the solution is suboptimal in this matter. I’m not fond of having to click through a dialog, but risking to miss some messages is not good either. The correct interface for non-fatal messages would be to pass them to the user session as notifications. However that would require revamping the notifications interface so that system notifications can be sent to users. That would be very useful for many things, but this is much broader than a GDM problem.
Review of attachment 187237 [details] [review]: Hey thanks for doing this. Overall it looks great. I have a few really minor nits that are more about "style" than anything else. One bigger issue though is we don't inhibit dialog resets in the event of authentication failure until after the last message gets its timeout. This means in some cases a message could still go by really quickly. ::: gdm-3.0.0.orig/gui/simple-greeter/gdm-greeter-login-window.c @@ +133,3 @@ guint start_session_handler_id; + + GSList *message_queue; I know it's a little heavier, but a GQueue feels more "right" to me here. @@ +239,3 @@ +{ + g_free (msg->message_text); + g_free (msg); should probably use g_slice_free for the QueuedMessage object @@ +253,3 @@ + +static gboolean +message_set_or_timeout (GdmGreeterLoginWindow *login_window) it's not really "or timeout" it's "or continue".. how about set_next_message_or_continue ? @@ +271,3 @@ + /* All messages have been shown, proceed */ + g_signal_emit (login_window, signals[START_SESSION], 0); + } So we're only deferring START_SESSION in the event of pending messages. We probably also need to defer dialog resets so people can see the last queued message on failure as well. @@ +282,3 @@ + gboolean needs_beep) +{ + QueuedMessage *msg = g_new0 (QueuedMessage, 1); again, g_slice (not because I think it matters in practice, but just because it's more the standard "pattern" these days) @@ +285,3 @@ + msg->message_text = g_strdup (text); + msg->timeout_seconds = timeout_seconds; + msg->needs_beep = needs_beep; Since we only ever have a small, finite set of "canned" configurations for beep and duration, I think it would make more sense to use a message type field here instead of separate items. then you could switch for the correct canned config at the time you play the message. That also let's us ditch the boolean argument from the function which is nicer for readability at the call points. @@ +836,3 @@ g_debug ("GdmGreeterLoginWindow: info: %s", text); + queue_message (GDM_GREETER_LOGIN_WINDOW (login_window), text, 2, FALSE); definitely want a #define instead of a hard coded number for the timeout. @@ +851,3 @@ maybe_show_cancel_button (login_window); + queue_message (GDM_GREETER_LOGIN_WINDOW (login_window), text, 3, TRUE); same deal with this number
I've made a few changes, which i'll post to the bug here for you guys to look at.
Created attachment 187988 [details] [review] greeter: add small delay when presenting messages When PAM sends a message up to the greeter to show the user, it shows it right away, immediately overwriting any previous message. This commit introduces a message queue, so that each pending message gets a reasonable amount of time on screen for the user to read.
Your patch seems to work fine for error messages displayed upon authentication failure, but warnings appearing after success (such as an expired password) don’t appear long enough, the session starts up immediately.
okay so the problem, I guess, is, we tell the slave to go ahead and log us in as soon as we get the nod that the user was authorized. At that point the message queue is empty so there's nothing to hold things up. The message is queued later, but the slave already got the go ahead from us to proceed so it kills us while we're in the process of showing the messages.
Created attachment 188082 [details] [review] greeter: add small delay when presenting messages When PAM sends a message up to the greeter to show the user, it shows it right away, immediately overwriting any previous message. This commit introduces a message queue, so that each pending message gets a reasonable amount of time on screen for the user to read. This iteration has small clean ups and a fix to delay START_SESSION• when there's only one message in flight.
Created attachment 188083 [details] [review] daemon,greeter: login after PAM has had time to talk to user The greeter currently gives the go ahead for the session to start as soon as the user has been authorized. Then the slave quickly runs through the remaining hoops and the greeter gets promptly killed. This commit changes the logic so that the slave doesn't get the go ahead to start the session until all the hoops have been run through, and potential messages have been queued, and displayed.
I think the two patches above should fix the remaining issues, but, as always, would appreciate review and testing feedback.
I have not checked all possibilities, but this version seems to work just fine with a pair of test cases.
Alright, I've pushed it to master now then. If any issues crop up, we can deal with them in subsequent commits.