Created attachment 126784 [details] [review]
Patch to allow gdm to work better with manual username entry

Note similar bug #528663.

Note there is a patch in bug #528663 which also tries to address the same problem, though without adding configuration options as done in this patch.

One issue with the patch in that bug is that GDM only shows the "Shutdown" and "Reboot" options when no user is selected in the face browser.  Does this patch address this issue and allow these buttons to be selected when the face browser is turned off?  You can read more about this issue in bug #528663.

Sorry I missed that ... I had searched all bugs from 2.24 on as I had mistakenly thought 2.24 was the first official version of the rewritten gdm. So I missed 528663 in the search.

With this patch I have tried to make the manual entry mechanism more exactly simulate the effect of clicking an entry in the face browser. We have shutdown/restart buttons disabled but I have turned them back on on a test machine and the behaviour is as follows:

The Shutdown and Restart buttons are shown at all times when the user is being prompted to select a user name. Once the greeter switches into Authentication mode after a user name has been entered, the Shutdown and Restart buttons are hidden.

For example:

   Scenario (1): When disable_user_list=true is set:

   The face browser is empty.
   The manual-user-entry area prompts the user to enter a user name.
   The Shutdown and Restart buttons are shown.

   As soon as the user enters a user name and presses Enter or clicks on
   the Begin button, the manual-user-entry area changes to a display
   of "Logging in as <username>", the Shutdown and Restart buttons are
   hidden, the Cancel button is activated, and the user is prompted for their
   password.

   If the user cancels, the display returns to the prompt to enter a user
   name, and the Shutdown and Restart buttons are shown again.

   Scenario (2):  When disable_user_list=false:

   The face browser is nonempty and contains an Other entry.
   The Shutdown and Restart buttons are shown.
   If the user clicks on "Other", the manual-user-entry area is
   activated, the Cancel button is activated, but the Shutdown and Restart
   buttons remain active as well.

   Should the user then manually enter a user name, the Shutdown and Restart
   buttons will become hidden as in scenario (1).

   If, however, the user presses Cancel,the manual-user-entry area is hidden,
   the face browser becomes fully expanded again, and the Shutdown and Restart
   buttons remain visible.

   Should the user click on a user name that is NOT Other, then the Shutdown
   and Restart buttons will be hidden and the user will be prompted to enter a
   password. In this case, the Cancel button will be active but not Shutdown
   or Restart.

   Again, should the user cancel, the Shutdown/Restart buttons will become
   active again.

I haven't done a very exhaustive test (since we have Shutdown/Restart always disabled here) but have tested the above scenarios and they behave as expected.

I have tested this patch and it works really well.  Much better than the approach that was being taken in the other bug report (#528663).  The shutdown and reboot
buttons work well with this patch, when they are enabled.

I'll make that bug a duplicate of this report, and I think this patch should be considered for upstream.  This patch makes turning off the face browser far more usable.

*** Bug 528663 has been marked as a duplicate of this bug. ***

The patch works very well in my gdm (2.24).
But there are three tiny problems.
1. in function set_user_selection_type,
   You did not hide user_chooser while using manual_user_chooser.
   So there is an extra seperate bar (bring by user_chooser)
2. the manual_user_chooser described in glade file was
   set below the caplock and message containers.
   So it seem inconsistent with the original designation.
   I donnot know if it was caused by my gdm version.
3. when the disable_user_list is not on, 
   manual_user_chooser will be showed at first and then be hided.
   It will also confuse the users.

  


(In reply to comment #1)
> Created an attachment (id=126784) [edit]
> Patch to allow gdm to work better with manual username entry

Regarding the three problems:

1. When I started to develop the patch, I tried hiding user_chooser. However, I found that the user chooser widget does some stuff with focus and events (stuff I don't completely understand) and the result is that the greeter crashes with a fatal error message "Widget not realized" if you try to hide user_chooser. So I thought the simplest solution was just to keep it shown, albeit empty (so all you see is that horizontal bar).

A better solution is probably for whoever wrote the user_chooser code, or some other gdm developer (who understands what it's trying to do), to adjust it to remove any 

  g_assert (GTK_WIDGET_REALIZED (widget))

tests and to wrap other calls that require realization to be inside an
  if (GTK_WIDGET_REALIZED(widget))
test.

Once that's done, then my patch can be modified by adding the following to the function set_user_selection_type:

if (selection_type == SELECTION_TYPE_MANUAL) 
           gtk_widget_hide(login_window->priv->user_chooser);
else
           gtk_widget_show(login_window->priv->user_chooser);

(And then that function's other call to gtk_widget_show(login_window->priv->user_chooser)
is redundant and can be removed)

2. It seems gdm-2.25 has removed the caps lock label altogether, with the comment that the caps lock warning is "sorta" handled automatically by Gtk+ now. So I guess the patch may need to be adjusted a bit when backported to 2.24 which does have the caps lock message in the glade file.

However, I'm finding (now that I try to test it) that I see no caps lock message at all. I can think of three possible reasons:
   - A bug in gdm-2.25 and/or whatever mechanism Gtk+ uses to display the
     caps lock warning
   - My patch broke something
   - The Gtk+ in the Fedora 10 system I'm using (1.2.10?) is too old to
     support the caps lock feature itself.

I suspect the third option, as Fedora 10 ships gdm 2.24 and I only went to 2.25 to fix 2.24's inability to act as an xdmcp server. But perhaps someone else can confirm?

3. I'm not sure why this is happening, since my patch tries to explicitly set SELECTION_TYPE_CHOOSER first, moving to SELECTION_TYPE_MANUAL only when the user chooser is empty after having being loaded.

But perhaps the dialog initialization is happening before the user list is even loaded -- in which case, the fix is probably to change the patch by making gdm_user_chooser_widget_is_empty return FALSE if the widget is not yet loaded (even though technically it is empty at that point!) Adding the following line to the start of gdm_user_chooser_widget_is_empty should do it:

if (! widget->priv->loaded) return FALSE; 

Created attachment 127369 [details] [review]
Alternative patch

I wasn't really fond of changing the glade file, so I took another try.

The basic idea is to monitor when there's only the Other user in the list, and when this is the case, we remove it from the list, but also we hide the list.

The other change is to show all buttons when the list is hidden. This fixes one of Ray's concerns, iirc.

I did only some rough testing, but it seems to work fine here.

I don't think you've followed the extent of the bug which the patch is designed to address, because your patch only solves the cosmetic issue of the sole "Other" button -- it doesn't address the larger issues, because it still tries to do a select_user(login_name) when login_name is "Other".

This means gdm is unable to properly select the REAL user, fails to preload the Session and Language menus based on the contents of the user's .dmrc file, and in short still has all the same problems as before other than the cosmetic ones.

The only way to get the Session and Language menus properly loaded is to do a select_user with the REAL user name before showing those menus. In other words, before switching to MODE_AUTHENTICATE, the user name needs to have been entered. That requires adding a new element to the glade file (or else manually creating a widget on the fly if you don't want to alter the glade file) so that the user has some means of entering a user name BEFORE the password authentication dialogue is shown. Just having the user name entered during the same pam conversation that prompts for the password, as done in your patch, does not work, because that conversation takes place well after the Language and Session menus have loaded.

However, one part of your patch that might be useful is the update_chooser_visibility part to hide just the scrollable widget portion of the chooser, if indeed that avoids the problems that occur when trying to hide the whole chooser.

Hmmm.  After reading the comment #10 I believe that the patch in comment #1 
will not work with alternative PAM modules.  Note that you cannot assume what 
prompts PAM will ask for.  You can not assume that it will even ask for a 
username at all.

For example, SmartCard or Fingerprint readers may not ask for username.  In
these cases, the username would be retrieved from the SmartCard or the 
fingerprint reader.  In other words, when the Face Browser is turned off, GDM 
should just blindly display whatever prompts PAM tells it to, and not assume 
username entry at all.

Note that the Face Browser is a bit of a PAM hack.  It only works with 
certain PAM modules.  In its defense, probably 99.9% of users tend to use PAM
modules which are compatible with the Face Browser.  So, it probably isn't
bad for GDM to assume that since most users tend to use Face Browser compatible
PAM modules, that the Face Browser feature should be on by default.  However, 
GDM should allow users to configure it to work with other types of PAM modules.

A better solution for the Shutdown/Reboot buttons would be to simply display
them for the first prompt and not display them on subsequent prompts, or to 
always display them for all prompts.

In terms of the Language/Session selection buttons, there are really two issues 
with how GDM currently handles these:

1) It would probably work best if the code retrieved the PAM_USER value from 
   PAM after each prompt and then display the Session/Language choice once a 
   username value exists.  The code should not assume that a username needs
   to be selected in a particular way, such as via the Face Browser or via
   a "username" specific prompt.  This would work better for a wider array
   of PAM modules, like ones where the username is retrieved via something
   like a SmartCard, but the user is still expected to enter a password.
   In such environments, a Face Browser would obviously not be desired since
   the idea of a SmartCard is to control access to the machines so that only
   people with a physical card can log in.

2) However there are probably some PAM environments where there is just one
   prompt.  Like a fingerprint reader that provides both username and password.
   So when the user puts their finger on the reader, they just log in
   immediately.  Such environments wouldn't be able to change their 
   language/session choices at all with the current code.  That seems a 
   problem.

   Another issue is that in some environments, it is important not to access 
   the users $HOME directory before authentication.  Note bug #155077 for an 
   example of this sort of issue.

   To address these types of issues, it might be necessary to add new 
   configuration settings to disable the feature of accessing the default 
   session/language values from the user's $HOME directory before 
   authentication and just always show the selection choices.  Not just 
   showing them after username entry.  Then users who want to use such PAM
   modules could just change the setting to make the language/session 
   entry choices available all the time without trying to get the default
   value from the $HOME directory first.

So, in short, I think the patch needs to be reworked so that when the face
browser is not shown, it just blindly displays the PAM prompts.  I think this
is how Vincent's patch works.  However, the code also needs to be smart enough
to handle the shutdown/reboot buttons sensibly.  Perhaps by just showing them
on the first prompt only.  Lastly, we need some sensible solution for showing
the language/session selection prompts.  For now, probably just making the
code work so it gets the username value from PAM_USER after each prompt and starts displaying the choices after there exists a username would be reasonable.
Fixing the code so that the session/language prompts can always be displayed
could probably be treated as a separate bug.

(gah, wasn't cc'ed on the bug)

(In reply to comment #11)
> So, in short, I think the patch needs to be reworked so that when the face
> browser is not shown, it just blindly displays the PAM prompts.  I think this
> is how Vincent's patch works.

(yes)

> However, the code also needs to be smart enough
> to handle the shutdown/reboot buttons sensibly.  Perhaps by just showing them
> on the first prompt only.

Well. It works fine the way it's done in the patch I submitted: if the user list is shown, you can see the shutdown/reboot when no user is selected; if the user list is not shown, then they are always displayed and it feels okay (at least for me).

> Lastly, we need some sensible solution for showing
> the language/session selection prompts.  For now, probably just making the
> code work so it gets the username value from PAM_USER after each prompt and
> starts displaying the choices after there exists a username would be
> reasonable.

Hrm. That would be a (minor) security issue, though, since it means you acknowledge this username is valid by showing his preferences (and this is something you might want to avoid, especially if the user list is disabled). I think in the case of prompting for a username, we should have "Last Layout", "Last Language" and "Last Session" options in the choosers.

Vincent, I just tested your patch and find that it works really well, as you describe.  The only usability issue I notice is that when the shutdown/reboot buttons are enabled you see both the "Restart" and "Cancel" buttons in the same row of buttons.  It isn't particularly clear whether "Restart" means to restart the login process or to restart the system.  So I could imagine users clicking on the wrong button.  Not sure how to best make this more usable.  Perhaps the buttons should not be in the same row or perhaps the shutdown/reboot buttons should move to the bottom panel?  Not sure.

However, that's a minor usability issue and the usability is 1000% times better with this patch than without.

> Hrm. That would be a (minor) security issue, though, since it means you
> acknowledge this username is valid by showing his preferences (and this is
> something you might want to avoid, especially if the user list is disabled).

I think you misunderstand me.  There isn't really a security issue with the
last session/language chooser dialogs.  Nor is this really related to this
bug report.  I was just talking.  Probably should file a separate bug report
about this.

But anyway, the problem is that if PAM doesn't prompt the user for anything 
(e.g. in the situation of a fingerprint reader or perhaps a Smart Card), then 
the user won't be prompted to change the session or language at all.  So,
for example, the user could never log into any other session except the
default one.

This is because the current code assumes that it should not show the dialogs until after a username has been entered, so it can probe for the user's default settings and show them.  Obviously if the PAM module won't ever prompt the
user at all, this assumption isn't really correct.

So, I was pointing out that for the new GDM to work with some PAM modules, it would probably be necessary to make GDM configurable so that the  language/chooser dialogs could be shown all the time, before
the user triggers the login event, thus allowing the user the ability to
change the settings if desired.

Created attachment 127510 [details] [review]
Updated alternative patch

There was a minor UI glitch on startup. This patch tries to fix this by showing the login window only when the user list is ready or after one second.

Created attachment 131842 [details] [review]
updated patch


Updating Vincent's patch so it applies to the latest GDM 2.26.0 release.

Vincent's patch looks good to me.  I've tested it and it works well, and seems to reasonably address the UI concerns.  Can this fix go upstream?

This patch does not work. Language and Session selection are completely broken:

 (1) the user's previous language and session settings are not remembered.

 (2) the language resets to the system default in between the entering of the
     login name and the password; however, despite this,

 (3) the language and session settings that were in effect BEFORE the username
     was entered are often used, ignoring any settings that were made after the
     username was entered and before the password was entered! This doesn't
     seem completely consistent, though; sometimes it seems to be the other
     way round, taking settings that were made after the username entry in
     preference to ones made before. I have not been able to figure out exactly
     when it uses the "before" settings and when it uses the "after" settings.

My original patch is the only one I have seen that provides working language and session selection. While it is true that it requires username entry even for PAM authentication methods that don't require it, that doesn't actually break anything -- unlike these later patches.

The only serious problem I've seen reported with my original patch is that it causes the search of a user's .dmrc file before the user has been authenticated, which I agree is a problem. But the latest proposed patches are not the solution.

If I have time I will try to work on adapting my original patch to integrate better with PAM's authentication mechanism and has an option to only load the .dmrc file after authentication.

Actually, looking at things a bit more closely, it seems to me that the latest patch SHOULD work better than it does ... there's something else going wrong.

Running with debugging enabled shows that, after the user name is entered, the greeter gets signals telling to change the session and language to those stored for that user.

However, for some reason it then gets a second set of signals telling it to change them back again. And that's (at least partly) what's preventing the patch from working the way it should. I'll see if I can figure out what's going on.

Created attachment 131874 [details] [review]
Fix to session worker so updated patch can user options properly

I've found the cause of why the most recent updated patch doesn't work. It's not actually a problem with the patch at all, but in gdm-session-worker.c: when the user enters the username during the PAM conversation, it FIRST reads the settings from the user's .dmrc file, THEN sends the "username changed" message. The username changed message causes all user settings to be reset to system default values, undoing the good of having read the .dmrc file. This patch corrects the problem by changing the order, so that FIRST the "username changed" message is sent, and THEN the panel settings are updated from the user's .dmrc file.

This patch in combination with the previous patch should provide a properly working gui when the face browser is disabled.

Hi Philip,

We actually already commited a patch very similiar to attachment 131874 [details] [review] a couple of weeks ago. See http://svn.gnome.org/viewvc/gdm/trunk/daemon/gdm-session-worker.c?r1=6794&r2=6793&pathrev=6794

Thanks very much!

With this fix, the patch by Vincent, as updated by Brian in attachment 131842 [details] [review], does indeed work well, just as Brian reported.

My apologies for initially denigrating this patch in comment #10 and comment #16. Based on my observations of gdm's behaviour, I had assumed it would only load the user's saved settings if the greeter emitted a "begin verification for user" signal rather than a plain "begin verification" signal. That was why my initial patch altered the structure of the code so as to delay sending any begin verification signal until after the username had been determined.

I now see that gdm is in fact designed to properly reread saved settings any time the username changes during the PAM conversation, and was only not doing so because of the bug in comment #18, which is now fixed. So that means the structural changes in my original patch are not necessary and the UI changes in Vincent's/Brian's patch work very well to solve the problems.

There's just one more small addition I'd like to make to the patch. As it stands, a user may try to set their language/session/keyboard settings before typing their user name (as was the custom in older gdm versions) then not notice it getting reset between the user name and password prompts. Because this is a probable cause for confusion and annoyance, I would like to see the user option menus hidden by default until the user name is determined -- except on systems that have a non-standard PAM setup where it needs to be shown during the whole conversation.

I will attach a patch to address this. The patch hides the user option menus until the username is known, unless the (new) boolean gconf key /apps/gdm/simple-greeter/show_user_options_early is set. That way, users of systems with a normal PAM setup don't get confused, and administrators of systems with a non-standard PAM can set the key to show the option menus during the entire conversation.

Created attachment 131926 [details] [review]
Patch to optionally hide user options until username is known

Created attachment 132283 [details] [review]
An alternative patch - see comments

After working with the current patch for a few days, I've noticed it does cause some problems because of the fact that it auto-initiates the PAM conversation, which will then time out and re-initiate itself after 10 minutes. The problems are:

   1. The computer will beep every 10 minutes; not too much of annoyance for a single machine but a significant one in a computer lab with dozens of machines!

   2. If a user happens to sit down at a computer at the end of a 10-minute cycle, they will see their first login attempt unexpectedly get cancelled and have to start over.

   3. We like to use gdm to show currently-logged-in users in the face browser (see bug #568326) which means that if a gdm flexiserver is running on one console while someone logs in on another, it may need to toggle between showing/hiding the "Other" user and it has to wait until the currently initiated conversation times out before it can do that.

But there's an alternative approach, that's actually a simpler patch, which avoids all these problems at the expense of a mildly more cumbersome user experience: instead of autostarting an "Other" login conversation when the chooser list is empty, simply enable the "Log In" button when the chooser list is empty, and have it start the PAM conversation when pressed.

The attached patch implements this. It also adds two additional features: (1) it includes patch #131926, and (2) it allows for the banner message to be worded differently when the chooser is present and visible from when it is empty and hidden (which I previously included as part of the patch in bug #568326 but moved here because it's touching the same files at the same places). Even with those new features it is still about the same size as the current patch. (If you strip it of those additional features it's significantly shorter). It also applies more cleanly to the version of gdm currently in Fedora rawhide, that has the massive "multistack.but.boring" patch (whereas the current patch does not apply without a fair bit of manual tweaking).

The only drawback of this alternative patch I'm proposing is that the user sees a screen with a single "Log In" button and has to press the button in order to get the prompt for their username and password. But I still think that's better than the original situation where they have to click on an item labelled "Other" where, even if it was labelled something more user-friendly, it still isn't visually apparent that it's something to click on, and it avoids the problems associated with auto-initiating a PAM conversation that then has to constantly keep timing out and restarting.

Why not just automatically start the login prompt if no users are logged in
rather than show the button to launch it?

The old GDM had code where you could set the list of users to a specific list of users, which might also be a different way to achieve a similar result.

> Why not just automatically start the login prompt if no users are logged in
> rather than show the button to launch it?

Because, as explained in comment #22, when you do that, (1) the computer will beep every 10 minutes, (2) a user who sits down at the end of that timeout period may get their login attempt unexpectedly cancelled while they are in the middle of typing, and (3) it makes it harder to implement a system where only logged-in users are shown, because it will need to wait until authentication is either timed out, cancelled, or finished before switching between chooser and chooserless modes.

You could solve problems (1) and (2) by eliminating the timeout on the PAM conversation, but I think that will bring problems of its own (such as, if authentication mechanisms change or users get added or removed, the conversation may not properly reflect the changes if it has been running for several days or weeks or months).

That's why I suggested the alternative patch. It's a compromise of course; slightly less convenient for the user, but it eliminates all the problems of automatically starting the login prompt.

I'd think fixing GDM to disable the beep in this case shouldn't be too hard of a fix.  

If you want a button to delay PAM starting until a user is actually at the terminal, then why not just run a zenity dialog from the Init script, so the user
is presented with a dialog and a button to press before GDM starts.  This is
the sort of purpose the Init script was designed for.  

I wouldn't think these features would require modifying the GDM source code, aside from perhaps disabling the beep on PAM restart.

Okay, I give up trying to explain it. I think the problems I've described are pretty obvious and go deeper than just "disabling a beep". But evidently I'm speaking to deaf ears.

We're already talking about modifying the gdm source code to hide the other user and automatically do "something" when the chooser list is empty.

If the "something" is auto-initiating the pam conversation, it is fairly complex and carries with it the problems I described.

If the "something" is merely showing a login button, it is simpler and eliminates those problems.

So I'm suggesting using the second "something" instead of the first. But now you're saying it's better to use the first, more complicated something and try to create elaborate workarounds to eliminate its problems?

An Init script wouldn't work at all. Init scripts are not rerun when the Cancel button is pressed, they are not run when the dialog mode is switched. And how in the world can an Init script no whether the gdm chooser list will be empty or not? Only gdm itself knows that!

My latest proposed patch works nicely. But it's obviously not wanted, so we'll just keep it as our own private fork -- feel free to close this bug report as a WONTFIX. I'm not going to spend any more time on this. We have it working nicely here, and if others aren't interested then fine.

Sorry, I don't mean to discourage your enthusiasm.  I have very much appreciated your input, actually.  I'm just trying to understand the problem.  I had assumed that you were using the button to delay the start of the PAM connection until a user was actually ready to login.  If that is the purpose, then using the Init script should work reasonably well.  If you run a program like a zenity dialog from the Init script then the launch of the GDM GUI will be delayed until that program completes.  

You are right that one disadvantage of using the Init script is that if a user starts to log-in and then stops, or hits the Cancel button, then this won't restart GDM, so the Init script won't be relaunched.  However, you could use GDM's autolaunch features to start a program which sends a HUP signal to the greeter after a timeout, so if the user doesn't login in 2 minutes or something, then it restarts the GUI with the Init script.  Yes, this is a bit
cumbersome.  However, displaying dialogs before the login screen is pretty
common since I often get questions from people about how to set this up.
Most typically people want some sort of click-through dialog that users must
confirm before they are allowed to login.  

I was just trying to suggest it as a workaround that doesn't require changing the code so much.  However, there's nothing wrong with adding options to GDM to 
make it easier to make it work, or to make it work in a specific way.  You
shouldn't think, based on my comments, that your patch is rejected in any way.
I'm just discussing the patch.

Perhaps it would be better to have a more general interface that solves a
wider set of problems.   So that instead of launching a button, you launch a 
script you can configure, perhaps to run zenity.  Then users could modify the 
script configuration and run what they want, which could be just an "OK" button 
in your case.

I've implemented #12 's suggestion that add "Last session" item.
Additionally, fix that the language will reset to ~/.dmrc if the selection is earlier than username entering when disable_user_list=true.

Created attachment 135923 [details] [review]
add "last*" items and fix language reset.

Okay, I'm confused.  What patches are needed to fix this bug?  I am guessing the patch in comment #15 plus the patch in comment #21 plus the patch in comment #29.  Is this correct, or is some other combination of patches needed to fix this bug?  It would be nice if someone could provide a single patch which resolves all the issues identified.

Sorry for late.
The problem I meet is that user want to select the right keyboard layout for entering user name. In this case, patch in comment #21 doesn't work.

Hi Philip,

I think starting the pam conversation late makes a lot of sense.

We don't want users logs filling up with failed pam conversations, we don't want fingerprint readers stuck on and burning out etc.

I'll have a look at your patch shortly, and sorry for the long delay.

Mingxi, can you file a separate bug report please?

Created attachment 138561 [details] [review]
Update to work with lastest master, drop gconf key to show panel items sooner

Hey, I fixed up the patch from attachment 132283 [details] [review] to work with master and made some minor changes:

1) fixed new code to match existing coding sytle
2) dropped the proposed gconf key to keep the old behavior of showing option widgets early. The new way make sense, we should just go with it.
3) renamed some variables
4) added back a removed grab_focus call
5) changed a signal from visibility-changed to a property named "list-visible"