GNOME Bugzilla – Bug 678057
support an initial-setup mode
Last modified: 2012-11-16 22:22:31 UTC
The https://live.gnome.org/ThreePointFive/Features/InitialSetup feature needs some support in gdm to run the initial setup app. The needed changes are (in a unrefined form) in the http://git.gnome.org/browse/gdm/log/?h=wip/initial-setup-redux branch
See bug 679474 for the current plans for how to do this.
Created attachment 219162 [details] [review] Update .gitignore
Created attachment 219163 [details] [review] libgdm: Make sure to install gdm-client-glue.h Otherwise, clients using libgdm will fail.
Created attachment 219164 [details] [review] daemon: Allow passing a username to SetupForProgram This allows the setup session to pass the gdm-initial-setup user.
Created attachment 219165 [details] [review] daemon: Allow setting the PAM_AUTHTOK for the login This lets us do an auto-login in the general case where we know the user's password, for gnome-initial-setup.
Created attachment 219166 [details] [review] daemon: Start replacing old method/signal-based API with async method calls Before, the session worker and session communicated by a series of signals and methods... but backwards. The session worker would listen for a series of signals sent out by the session, and respond back by calling methods on it. This requires a lot of annoying, silly manual labor when trying to add another method to the API. For now, just replace three method/signal API pairs on the worker that are a part of the state machine, and leave the rest of the cleanup for another day.
Created attachment 219167 [details] [review] daemon: Allow starting a transient program The setup session will need to start a transient program before the actual session runs, in order to run a process that copies all data from the gdm-initial-setup user to the user's directory. Give it an API to do so that it can call. Nothing uses it quite yet, though...
Created attachment 219168 [details] [review] daemon: Remove GdmGreeterSession and GdmChooserSession With the recent cleanup, these two are just dummy class subtypes. Duplicating this one more time for the setup session wouldn't be worth it. Into the trash it goes.
Created attachment 219169 [details] [review] daemon: Remove unused GdmWelcomeSession properties These weren't used, either in the implementation, nor the interface.
Created attachment 219170 [details] [review] daemon: Rename GdmWelcomeSession to GdmProgramSpawner "GdmWelcomeSession" was always a sort of bad name, as it is not a GdmSession, itself (it has-a GdmSession), and it's unnecessarily generic; it doesn't really have anything to do with "Welcome" or "Session" itself. It managed a non-user GdmSession, spawned the process in the correct environment (spawning a DBus daemon if need be) and made sure to keep track of it until it died. I think "GdmProgramSpawner" is an appropriate name for this.
Created attachment 219171 [details] [review] daemon: Add initial setup integration
I committed a few of these before my brown bag 3.5.4.1 release today
Comment on attachment 219166 [details] [review] daemon: Start replacing old method/signal-based API with async method calls This probably needs to be more exhaustive before it can go in.
Review of attachment 219165 [details] [review]: ::: daemon/gdm-session-worker.c @@ +2657,3 @@ + GdmSessionWorker *worker) +{ + worker->priv->pam_authtok = g_strdup (pam_authtok); I'd call pam_set_item directly here. I wouldn't defer to do_setup. I don't think it's a good idea to keep the password sitting around, so I wouldn't keep it in the worker priv struct. Also, I think this function needs to ensure the current state is after SETUP and before AUTHENTICATE. ::: daemon/gdm-session.xml @@ +116,3 @@ <arg name="display_is_local" type="b"/> </signal> + <signal name="SetPamAuthTok"> The inner details of pam and even the word pam are fairly deliberately abstracted away in the API. The closest existing interfaces we have to this are SecretInfoQuery and AnswerQuery. How about SetInitialSecret ?
Review of attachment 219170 [details] [review]: So the more I think about this, I think GdmProgramSpawner isn't a great fit either. Like you said, it does a lot more than spawn a program. It sets up a whole environment for the program to run in (including dbus, and environment variables for that environment etc). How about GdmLaunchEnvironment ?
Created attachment 219264 [details] [review] daemon: Allow setting the PAM_AUTHTOK for the login This lets us do an auto-login in the general case where we know the user's password, for gnome-initial-setup.
Created attachment 219265 [details] [review] daemon: Start replacing old method/signal-based API with async method calls Before, the session worker and session communicated by a series of signals and methods... but backwards. The session worker would listen for a series of signals sent out by the session, and respond back by calling methods on it. This requires a lot of annoying, silly manual labor when trying to add another method to the API. For now, just replace three method/signal API pairs on the worker that are a part of the state machine, and leave the rest of the cleanup for another day.
Created attachment 219266 [details] [review] daemon: Allow starting a transient program The setup session will need to start a transient program before the actual session runs, in order to run a process that copies all data from the gdm-initial-setup user to the user's directory. Give it an API to do so that it can call. Nothing uses it quite yet, though...
Created attachment 219267 [details] [review] daemon: Rename GdmWelcomeSession to GdmLaunchEnvironment "GdmWelcomeSession" was always a sort of bad name, as it is not a GdmSession, itself (it has-a GdmSession), and it's unnecessarily generic; it doesn't really have anything to do with "Welcome" or "Session" itself. It managed a non-user GdmSession, spawned the process in the correct environment (spawning a DBus daemon if need be) and made sure to keep track of it until it died. I think "GdmLaunchEnvironment" is an appropriate name for this.
Created attachment 219268 [details] [review] daemon: Add initial setup integration
Created attachment 219269 [details] [review] simple-greeter: Fix warnings
While we would like to have the async method patch to be finished too, it's just a lot of manual labor, so I'd like some feedback on the approach before we convert everything.
Review of attachment 219267 [details] [review]: My only complaint is the spacing gets all messed up by the rename. Other than that, it's fine.
Comment on attachment 219269 [details] [review] simple-greeter: Fix warnings Attachment 219269 [details] pushed as a071378 - simple-greeter: Fix warnings
Created attachment 219271 [details] [review] daemon: Remove dummy constructor/property functions They don't do anything.
Review of attachment 219264 [details] [review]: Commit message should be something like: daemon: Provide mechanism for providing authentication secret up front Some PAM modules can be told their password ahead of time to preventing them having to ask later. This is accomplished by setting the PAM_AUTHTOK item before calling pam_authenticate. gnome-initial-setup needs to be able to do this to reuse the username/password entered at the user creation/enrollment page. This commit enables this capability with a new "blah" interface. ::: daemon/gdm-session-worker.c @@ +2656,3 @@ + GdmSessionWorker *worker) +{ + worker->priv->initial_secret = g_strdup (initial_secret); I think you should call pam_set_item here after ensuring the state is SETUP_COMPLETE ::: daemon/gdm-session.c @@ +1433,3 @@ + + conversation = find_conversation_by_name (self, service_name); + need to check null here. in fact, it will always be null right? conversations are started implicitly at BeginVerification time, so I don't think this can work. I think instead you'll need one or two more WithInitialSecret variants to BeginVerification
Review of attachment 219265 [details] [review]: general approach seems fine, and more logical than what we're doing now. You should make sure there's an unlimited timeout for the method calls to reply, and that everything is ported over to the new style. ::: daemon/gdm-session-worker.c @@ +161,3 @@ GdmSessionSettings *user_settings; + + GDBusMethodInvocation *invocation; I'd probably add the prefix pending_ to this variable, but that's just me, do whatever. @@ +2745,3 @@ + get_state_name (new_state), + get_state_name (worker->priv->state + 1)); + return FALSE; You should return TRUE even for errors. FALSE means "i haven't handled this, let the next person who has connected to the signal give it a go." @@ +2767,3 @@ +gdm_session_worker_handle_authenticate (GdmDBusWorker *object, + GDBusMethodInvocation *invocation, + const gchar *arg_service_name) just use const char *service_name, instead of const gchar *service_name, @@ +2789,3 @@ + GdmSessionWorker *worker = GDM_SESSION_WORKER (object); + if (!validate_and_queue_state_change (worker, invocation, GDM_SESSION_WORKER_STATE_ACCREDITED)) + return FALSE; gdm codebase tries to alway use braces for if statements @@ +2869,3 @@ + GDM_WORKER_DBUS_NAME, + G_BUS_NAME_OWNER_FLAGS_NONE, + NULL, NULL, NULL, NULL); You don't need to take a name. Just pass NULL for the name on the other end of the pipe. ::: daemon/gdm-session.c @@ +326,3 @@ +static void +on_authenticate_cb (GdmDBusWorker *proxy, using past-tense is more common in GDM than _cb, so e.g. on_authenticated ::: daemon/gdm-session.xml @@ +1,3 @@ <!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN" "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd"> <node name="/org/gnome/DisplayManager/Session"> + <interface name="org.gnome.DisplayManager.Worker"> Shouldn't this interface be on a separate /org/gnome/DisplayManager/SessionServer node ?
Created attachment 219297 [details] [review] daemon: Fix error handling in BeginVerification and siblings We were trying to return an error on the wrong invocation.
Created attachment 219298 [details] [review] daemon: Refactor BeginVerification and siblings As we're going to add a new variant that additionally asks for a secret, we need to do this.
Created attachment 219299 [details] [review] daemon: Provide mechanism for providing authentication secret up front Some PAM modules can be told their password ahead of time to preventing them having to ask later. This is accomplished by setting the PAM_AUTHTOK item before calling pam_authenticate.
Created attachment 219300 [details] [review] daemon: Start replacing old method/signal-based API with async method calls Before, the session worker and session communicated by a series of signals and methods... but backwards. The session worker would listen for a series of signals sent out by the session, and respond back by calling methods on it. This requires a lot of annoying, silly manual labor when trying to add another method to the API. For now, just replace three method/signal API pairs on the worker that are a part of the state machine, and leave the rest of the cleanup for another day.
Created attachment 219301 [details] [review] daemon: Allow starting a transient program The setup session will need to start a transient program before the actual session runs, in order to run a process that copies all data from the gdm-initial-setup user to the user's directory. Give it an API to do so that it can call. Nothing uses it quite yet, though...
Created attachment 219302 [details] [review] daemon: Rename GdmWelcomeSession to GdmLaunchEnvironment "GdmWelcomeSession" was always a sort of bad name, as it is not a GdmSession, itself (it has-a GdmSession), and it's unnecessarily generic; it doesn't really have anything to do with "Welcome" or "Session" itself. It managed a non-user GdmSession, spawned the process in the correct environment (spawning a DBus daemon if need be) and made sure to keep track of it until it died. I think "GdmLaunchEnvironment" is an appropriate name for this.
Created attachment 219303 [details] [review] daemon: Add initial setup integration
Created attachment 219304 [details] [review] daemon: Remove dummy constructor/property functions They don't do anything.
Created attachment 219305 [details] [review] daemon: Debug message fixes
Review of attachment 219297 [details] [review]: doh, error handling hard.
Review of attachment 219298 [details] [review]: sure
Review of attachment 219305 [details] [review]: ::: daemon/gdm-session.c @@ +2256,3 @@ } + g_debug ("GdmSession: Beginning setup for session for program with log '%s' %s", log_file, service_name); second %s should probably have a "using PAM service " before it or so
Attachment 219297 [details] pushed as 48d2e54 - daemon: Fix error handling in BeginVerification and siblings Attachment 219298 [details] pushed as 416f7e6 - daemon: Refactor BeginVerification and siblings Attachment 219304 [details] pushed as fc276c4 - daemon: Remove dummy constructor/property functions Attachment 219305 [details] pushed as fec3124 - daemon: Debug message fixes
Review of attachment 219299 [details] [review]: commit message has "preventing them having" instead of "prevent them from having" (my fault). Looks good. Just needs a few small changes. ::: daemon/gdm-session.c @@ +75,3 @@ GDBusMethodInvocation *starting_invocation; char *starting_username; + char *secret; since the other two state variables used at the same time have a starting_ prefix, secret should probably be called starting_secret @@ +1182,3 @@ + if (conversation->secret != NULL) { + gdm_dbus_worker_manager_emit_set_initial_secret (worker_manager_interface, + conversation->secret); needs a g_clear_pointer here to clean it up. Also, probably needs to get cleared in the error case for gdm_session_start_conversation
Review of attachment 219300 [details] [review]: So again, I'm fine with this, but only if you go all the way. I don't want a mixture of old-school-dbus-way and new way.
Review of attachment 219301 [details] [review]: ::: daemon/gdm-session-worker.c @@ +2422,3 @@ +parse_start_program (GdmSessionWorker *worker, + char ***argv, + const char *text) The name "parse_start_program" doesn't really describe what this function does. One other thing is, I'd suggest putting out arguments at the end of the argument list. Though, this function does two things: 1) validates the worker is in a state where it can start the program 2) converts the command to an arg array Those things are actually conceptually separate enough, I'm not sure I would even put them in the same function. @@ +2480,3 @@ + pid = gdm_session_execute (argv[0], argv, + gdm_session_worker_get_environment (worker), + TRUE); gdm_session_execute doesn't return; it transmutates the currently running process into the new process. You need a fork() somewhere, to give it a sacrificial child to execute.
Review of attachment 219302 [details] [review]: overall looks fine. The spacing got messed up by your rename. I know it's pain, but I'd appreciate it if you went through and realigned all the function signatures and multi-line function calls to keep things tidy. The other thing is I think you forgot to "git mv gdm-welcome.pam gdm-launch-environment.pam" since I don't see them in your patch ::: daemon/gdm-xdmcp-chooser-slave.c @@ +65,3 @@ guint connection_attempts; + GdmLaunchEnvironment *chooser; don't you want to rename this chooser_environment too, like you renamed greeter to greeter_environment ? (i don't care either way)
Review of attachment 219303 [details] [review]: commit message should describe briefly what initial setup is, point to the wiki page, and basically just give a small contained overview of the feature. ::: daemon/gdm-simple-slave.c @@ +851,2 @@ static GdmLaunchEnvironment * +create_environment_for (const char *session_id, not sure about the _for suffix is it for a session? for a user? for a display? for a seat? I'd just elide the _for @@ +859,3 @@ { gboolean debug = FALSE; + char *command_base; const char * (I know it was char * before) @@ +877,1 @@ } One idea is you could GPtrArray *args; args = g_ptr_array_new(); g_array_add(args, BINDIR "/gnome-session g_ptr_array_add(args, "-f"); if (debug) g_ptr_array_add (args, "--debug"); if (session_id != NULL) { ... } etc g_ptr_array_add(args, NULL); command = g_strjoinv (" ", g_ptr_array_free (args, FALSE)); Just an idea. Do whatever you like better. @@ +947,2 @@ g_debug ("GdmSimpleSlave: Creating greeter on %s %s %s", display_name, display_device, display_hostname); + slave->priv->greeter_environment = create_environment_for (NULL, shouldn't this be session_id ? Another thought: the name session_id is sort of loaded, unfortunately. We have gnome-session's session, gdm's xsession name, logind's user session id all in play. Might make sense to call this gnome_session_id. ANOTHER THOUGHT, have a separate dconf profile for initial setup and then pass that in instead of session_id. It would change change what we cause the g_hash_table_insert (hash, g_strdup ("DCONF_PROFILE"), g_strdup ("gdm")) to get changed. Then you could just put your session in dconf like we do currently for the greeter session @@ +1107,3 @@ + +static void +start_session_timed (GdmSimpleSlave *slave) maybe call this start_greeter_or_autologin @@ +1115,3 @@ + + setup_server (slave); + Though, don't you need to call setup_server() and create_new_session() in the start_initial_setup case too? Maybe you should merge this with its caller, so its setup_server(); if (wants_initial_setup) start_initial_setup(); else if (!autologin) { start_greeter (); } else { run_script ("/Init"); } create_new_session (); @@ +1125,3 @@ + /* Run the init script. gdmslave suspends until script has terminated */ + gdm_slave_run_script (GDM_SLAVE (slave), GDMCONFDIR "/Init", GDM_USERNAME); + reset_session (slave); there was an upstream fix to this code recently, you'll need to rebase. (reset_session should be create_new_session ()) @@ +1136,3 @@ + if (!g_file_test ("/run/wants-initial-setup", G_FILE_TEST_EXISTS)) + return FALSE; + if statements are always supposed to have braces in GDM code.
Created attachment 219553 [details] [review] daemon: Clean up memory leaks We need to make sure we free these dup'd strings so that we do not leak private information or memory.
Created attachment 219554 [details] [review] daemon: Remove old, unused signals These are all dead signals that never got removed.
Created attachment 219555 [details] [review] daemon: Start replacing old method/signal-based API with async method calls Before, the session worker and session communicated by a series of signals and methods... but backwards. The session worker would listen for a series of signals sent out by the session, and respond back by calling methods on it. This requires a lot of annoying, silly manual labor when trying to add another method to the API. So, reverse the API so that the worker manager calls async methods on the worker itself.
Created attachment 219556 [details] [review] daemon: Provide mechanism for providing an authentication secret up front Some PAM modules can be told their password ahead of time to preventing them having to ask later. This is accomplished by setting the PAM_AUTHTOK item before calling pam_authenticate.
Created attachment 219557 [details] [review] daemon: Allow starting a transient program The setup session will need to start a transient program before the actual session runs, in order to run a process that copies all data from the gdm-initial-setup user to the user's directory. Give it an API to do so that it can call. Nothing uses it quite yet, though...
Created attachment 219558 [details] [review] daemon: Rename GdmWelcomeSession to GdmLaunchEnvironment "GdmWelcomeSession" was always a sort of bad name, as it is not a GdmSession, itself (it has-a GdmSession), and it's unnecessarily generic; it doesn't really have anything to do with "Welcome" or "Session" itself. It managed a non-user GdmSession, spawned the process in the correct environment (spawning a DBus daemon if need be) and made sure to keep track of it until it died. I think "GdmLaunchEnvironment" is an appropriate name for this.
Created attachment 219559 [details] [review] daemon: Add initial setup integration
Created attachment 219560 [details] [review] add clear_pointer xxx for live installs
Created attachment 219561 [details] [review] kenny loggins
Review of attachment 219561 [details] [review]: did not mean to attach this
Review of attachment 219560 [details] [review]: did not mean to attach this
Review of attachment 219557 [details] [review]: This one is still wrong. I want to play around with scrapping the special exec code and just use gspawn directly.
Review of attachment 219553 [details] [review]: ::: daemon/gdm-session.c @@ +1823,3 @@ } + g_clear_pointer (&conversation->service_name, (GDestroyNotify) g_free); This one is already done a few lines lower @@ +1824,3 @@ + g_clear_pointer (&conversation->service_name, (GDestroyNotify) g_free); + g_clear_pointer (&conversation->starting_username, (GDestroyNotify) g_free); This one should normally be freed as soon as the worker chimes in, but I guess the worker could potentially get stopped before it chimes in in some cases, so this fix makes sense. @@ +1826,3 @@ + g_clear_pointer (&conversation->starting_username, (GDestroyNotify) g_free); + g_clear_pointer (&conversation->session_id, (GDestroyNotify) g_free); + looks good.
Review of attachment 219554 [details] [review]: looks mostly fine. ::: daemon/gdm-session.xml @@ -148,3 @@ - <signal name="OpenSession"> - <arg name="service_name" type="s"/> - </signal> we still emit this one from gdm_session_open_session, so you'll need to drop that function too.
Review of attachment 219555 [details] [review]: So this seems like a good clean up, but I don't understand why there is still a worker manager interface. If we're reversing things and adding a new worker interface, why are we keeping the worker manager interface ? You should do this all the way.
Review of attachment 219556 [details] [review]: Description still says "preventing", otherwise seems fine.
Review of attachment 219558 [details] [review]: same comments as before: "spacing", and "do you want to rename chooser to chooser_environment, like you renamed greeter to greeter_environment ?"
Review of attachment 219559 [details] [review]: Same comments as before.
Review of attachment 219555 [details] [review]: Because the four PAM interfaces should be async methods that the worker calls, unless we want to pull the same method/signal hacks on the greeter itself. Reauth is hard as well.
Review of attachment 219555 [details] [review]: Well, there's the gdm-session.c middle man, so for Info, InfoQuery, SecretInfo, and SecretInfoQuery you could switch them to signals without affecting the greeter, but you're right, I agree it doesn't make sense for those 4 (the whole point is to try to get away from using signals for method calls after all). Still, why isn't ServiceUnavailable a signal, SetupComplete/SetupFailed dropped in favor of replies to Setup* calls. What makes reauthentication hard? I'd expect StartReauthentication to be a method call, ReauthenticationStarted dropped in favor of a reply to that call, and Reauthenticated becoming a signal.
(In reply to comment #67) > Review of attachment 219555 [details] [review]: > > SetupComplete/SetupFailed dropped in favor of replies to Setup* calls The code path we take doesn't currently vary the complete_* method calls back, but I suppose it doesn't actually matter.
yea you can just use g_dbus_method_invocation_return_value (just like if you have an error you have to use g_dbus_method_invocation_return_error and not a generated function)
Review of attachment 219557 [details] [review]: This won't be needed.
Attachment 219553 [details] pushed as a19d6d8 - daemon: Clean up memory leaks Attachment 219554 [details] pushed as ae725b0 - daemon: Remove old, unused signals
Created attachment 220012 [details] [review] daemon: Start replacing old method/signal-based API with async method calls Before, the session worker and session communicated by a series of signals and methods... but backwards. The session worker would listen for a series of signals sent out by the session, and respond back by calling methods on it. This requires a lot of annoying, silly manual labor when trying to add another method to the API. So, reverse the API so that the worker manager calls async methods on the worker itself.
Created attachment 220013 [details] [review] daemon: Provide mechanism for providing an authentication secret up front Some PAM modules can be told their password ahead of time to prevent them having to ask later. This is accomplished by setting the PAM_AUTHTOK item before calling pam_authenticate.
Created attachment 220014 [details] [review] daemon: Rename GdmWelcomeSession to GdmLaunchEnvironment "GdmWelcomeSession" was always a sort of bad name, as it is not a GdmSession, itself (it has-a GdmSession), and it's unnecessarily generic; it doesn't really have anything to do with "Welcome" or "Session" itself. It managed a non-user GdmSession, spawned the process in the correct environment (spawning a DBus daemon if need be) and made sure to keep track of it until it died. I think "GdmLaunchEnvironment" is an appropriate name for this.
Created attachment 220015 [details] [review] daemon: Add initial setup integration When a system boots for the first time, we want to show a special tool that will allow the user to set up their system the way they want to. This will be triggered by a special file: /var/run/wants-initial-setup The responsibilities of this tool include creating the user's account, so we have to create a special user account to run the tool under. Administrators are given the ability to turn this off in a GDM setting if they want to.
OK, this should address all of the review comments. provided I haven't forgotten one.
Review of attachment 220014 [details] [review]: Feel free to commit this one after considering what i mention below. ::: daemon/gdm-welcome-session.c @@ +107,3 @@ +static void gdm_launch_environment_class_init (GdmLaunchEnvironmentClass *klass); +static void gdm_launch_environment_init (GdmLaunchEnvironment *launch_environment); +static void gdm_launch_environment_finalize (GObject *object); spacing @@ +777,3 @@ * @disp: Pointer to a GdmDisplay structure * + * Starts a local X launch_environment. Handles retries and fatal errors properly. X launch_environment sounds weird so you could clean it up if you wanted (but no biggie either way) ::: daemon/gdm-simple-slave.c @@ +799,3 @@ static void +on_greeter_environment_session_opened (GdmLaunchEnvironment *greeter, + GdmSimpleSlave *slave) spacing, also maybe s/greeter/greeter_environment/ ? @@ +812,3 @@ static void +on_greeter_environment_session_started (GdmLaunchEnvironment *greeter, + GdmSimpleSlave *slave) likewise @@ +819,3 @@ static void +on_greeter_environment_session_stopped (GdmLaunchEnvironment *greeter, + GdmSimpleSlave *slave) likewise @@ +834,2 @@ static void +on_greeter_environment_session_exited (GdmLaunchEnvironment *greeter, likewise @@ +845,2 @@ static void +on_greeter_environment_session_died (GdmLaunchEnvironment *greeter, likewise ::: data/Makefile.am @@ +92,3 @@ pam_redhat_files = pam-redhat/gdm.pam \ pam-redhat/gdm-autologin.pam \ + pam-redhat/gdm-launch-environment.pam \ Still forgot to git mv this.
Review of attachment 220012 [details] [review]: Commit message says "Start replacing" but should say "Replace". The patch didn't apply because of StartConversation still being in the xml file. Seems to work in brief (not exhaustive) testing. go for it. ::: daemon/gdm-session-worker.c @@ +2105,3 @@ GDM_SESSION_WORKER_ERROR_SERVICE_UNAVAILABLE)) { + gdm_dbus_worker_emit_service_unavailable (GDM_DBUS_WORKER (worker), + error->message); You need to g_dbus_method_invocation_return_value (worker->priv->pending_invocation, NULL); here to reply and free the invocation. Though, maybe it would be better to drop the ServiceUnavailable signal and instead just make it a separate error code. Either way. @@ +2123,3 @@ G_CALLBACK (on_saved_language_name_read), worker); + worker->priv->pending_invocation = NULL; should do this immediately after each g_dbus_method_invocation_return_* call so we don't have dangling pointers. @@ +2154,3 @@ g_debug ("GdmSessionWorker: Unable to use authentication service"); + gdm_dbus_worker_emit_service_unavailable (GDM_DBUS_WORKER (worker), + error->message); same as above ::: daemon/gdm-session-worker.xml @@ +1,3 @@ +<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN" "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd"> +<node name="/org/gnome/DisplayManager/SessionServer"> + <interface name="org.gnome.DisplayManager.Worker"> It's a little strange the interface is "Worker" and the object is "SessionServer". It might make sense to use the same name for both. I'd vote for ditching SessionServer since we no longer user the BlahServer lingo anywhere else (we dropped GreeterServer, ChooserServer, etc)
Review of attachment 220013 [details] [review]: looks fine.
Review of attachment 220015 [details] [review]: ::: daemon/gdm-simple-slave.c @@ +981,2 @@ } need a g_ptr_array_add (args, NULL); here to NULL terminate the list @@ +1148,3 @@ + if (preexisting_user) { + if (!g_file_test (dest_filename, G_FILE_TEST_EXISTS)) { + g_warning ("User '%s' exists, but file '%s' doesn't", username, dest_filename); maybe it should just put the file in place instead of failing? I can imagine the user accidentally kicking the plug at exactly the wrong moment here and then ending up in a state where there computer doesn't work anymore. It's kind of a tough question, because we need to be cautious about doling out the policy since it gives extra privileges @@ +1168,3 @@ + g_free (contents); + return FALSE; + } This is fine, though g_file_copy might be better? @@ +1227,3 @@ + gboolean enabled; + + if (!g_file_test ("/var/run/wants-initial-setup", G_FILE_TEST_EXISTS)) { /var/run is a symlink to /run which is a ram disk. That means it would be hard to actually put the file in place and have it stick around through reboots. you probably want /var/lib/gdm/run-initial-setup or /var/spool/gdm/run-initial-setup @@ +1257,3 @@ + if (wants_initial_setup (slave)) { + start_initial_setup (slave); + } else if (!wants_autologin (slave)) { should be wants_autologin, not !wants_autologin @@ +1260,3 @@ /* Run the init script. gdmslave suspends until script has terminated */ gdm_slave_run_script (GDM_SLAVE (slave), GDMCONFDIR "/Init", GDM_USERNAME); + reset_session (slave); as mentioned earlier, this is a hold over from a bug fixed in git. you need create_new_session() not reset_session().
(In reply to comment #77) > Still forgot to git mv this. I didn't; look at the raw patch. For some reason Splinter doesn't show it, though.
Created attachment 220024 [details] [review] daemon: Replace old method/signal-based API with async method calls Before, the session worker and session communicated by a series of signals and methods... but backwards. The session worker would listen for a series of signals sent out by the session, and respond back by calling methods on it. This requires a lot of annoying, silly manual labor when trying to add another method to the API. So, reverse the API so that the worker manager calls async methods on the worker itself.
Created attachment 220025 [details] [review] daemon: Provide mechanism for providing an authentication secret up front Some PAM modules can be told their password ahead of time to prevent them having to ask later. This is accomplished by setting the PAM_AUTHTOK item before calling pam_authenticate.
Created attachment 220026 [details] [review] daemon: Rename GdmWelcomeSession to GdmLaunchEnvironment "GdmWelcomeSession" was always a sort of bad name, as it is not a GdmSession, itself (it has-a GdmSession), and it's unnecessarily generic; it doesn't really have anything to do with "Welcome" or "Session" itself. It managed a non-user GdmSession, spawned the process in the correct environment (spawning a DBus daemon if need be) and made sure to keep track of it until it died. I think "GdmLaunchEnvironment" is an appropriate name for this.
Created attachment 220027 [details] [review] daemon: Add initial setup integration When a system boots for the first time, we want to show a special tool that will allow the user to set up their system the way they want to. This will be triggered by a special file: /var/run/wants-initial-setup The responsibilities of this tool include creating the user's account, so we have to create a special user account to run the tool under. Administrators are given the ability to turn this off in a GDM setting if they want to.
Review of attachment 220026 [details] [review]: Assuming this is the same as before (but maybe with spacing fix ups), so just transfering the acn
Review of attachment 220027 [details] [review]: commmit message references /var/run. ::: daemon/gdm-simple-slave.c @@ +1005,3 @@ +start_launch_environment (GdmSimpleSlave *slave, + char *username, + char *session_id) So you decided to stick with the session_id argument instead of my DCONF_PROFILE alternative (which is fine). It might make sense at some point to rename /etc/dconf/db/gdm to something more generic-y so the environment variable isn't DCONF_PROFILE=gdm for gnome-initial-setup but on the other hand gdm is still behind launching gnome-initial-setup so maybe there's nothing wrong with keeping things exactly like they are. @@ +1212,3 @@ + +#define INITIAL_SETUP_TRIGGER_FILE LOCALSTATEDIR "gdm/run-initial-setup" + LOCALSTATEDIR "lib/gdm/run-initial-setup" or LOCALSTATEDIR "spool/gdm/run-initial-setup"
Review of attachment 220024 [details] [review]: ::: daemon/Makefile.am @@ +160,3 @@ gdm-session-record.h \ + gdm-session-worker-common.c \ + gdm-session-worker-common.h \ Why did you split this out?
(In reply to comment #88) > Why did you split this out? The error quark is in gdm-session-worker.c, which I can't include in the slave.
Review of attachment 220024 [details] [review]: ::: daemon/gdm-session.c @@ +291,3 @@ + GDM_SESSION_WORKER_ERROR, + GDM_SESSION_WORKER_ERROR_SERVICE_UNAVAILABLE)); + } if you call g_error_matches like this, then I think you need to call g_dbus_error_register_error_domain in gdm_session_worker_error_quark. I did some hack here to make it more automatic (see the register_error_domain function) register_error_domain But that's a bit over the top. Might be easier to just do the g_strcmp0 thing you do for accounts service in the initial-setup patch. Or just punt and keep things the way they were before with the separate signal.
umm not sure what happened in the last comment. Above where it says: register_error_domain It should say http://git.gnome.org/browse/gnome-settings-daemon/tree/plugins/identity/gsd-identity-service.c?h=wip/identity#n579 (not necessarily recommending that approach, just pointing it out as code i wrote recently). Honestly, I'd probably just punt back to the way it was before comment 78 at this point, doesn't seem worth the increase in effort.
Created attachment 220031 [details] [review] daemon: Replace old method/signal-based API with async method calls Before, the session worker and session communicated by a series of signals and methods... but backwards. The session worker would listen for a series of signals sent out by the session, and respond back by calling methods on it. This requires a lot of annoying, silly manual labor when trying to add another method to the API. So, reverse the API so that the worker manager calls async methods on the worker itself.
Review of attachment 220031 [details] [review]: seems fine, nice clean up overall. A few comments below, but otherwise ready to commit I think. ::: daemon/gdm-session-worker-common.c @@ +53,3 @@ + gdm_session_worker_error_entries, + G_N_ELEMENTS (gdm_session_worker_error_entries)); + G_STATIC_ASSERT (G_N_ELEMENTS (gdm_session_worker_error_entries) - 1 == GDM_SESSION_WORKER_ERROR_IN_REAUTH_SESSION); What's the point of this assert? It doesn't actuallly protect against anything does it? It would be useful to try to catch the enum growing without the mapping getting updated, but this doesn't achieve that. If that's what you're after, you could add a GDM_SESSION_WORKER_NUMBER_OF_ERRORS entry to the bottom of the enum and then G_STATIC_ASSERT (GDM_SESSION_WORKER_NUMBER_OF_ERRORS - 1 == GDM_SESSION_WORKER_ERROR_IN_REAUTH_SESSION); though that might muck up introspection data. would be neat if there was a gcc extension that would give the range of an enum. Anyway, I'd probably just drop the ASSERT unless you have a good reason for it ::: daemon/gdm-session-worker.c @@ +2093,1 @@ g_error_free (error); you could combine these two lines into g_dbus_method_invocation_take_error if you wanted ::: daemon/gdm-session.c @@ +779,3 @@ + } else { + report_problem_and_stop_conversation (self, service_name, error->message); + gdm_session_stop_conversation (self, service_name); This gdm_session_stop_conversation is redundant I think after report_problem_and_stop_conversation
(In reply to comment #93) > Review of attachment 220031 [details] [review]: > > seems fine, nice clean up overall. A few comments below, but otherwise ready to > commit I think. > > ::: daemon/gdm-session-worker-common.c > @@ +53,3 @@ > + gdm_session_worker_error_entries, > + G_N_ELEMENTS > (gdm_session_worker_error_entries)); > + G_STATIC_ASSERT (G_N_ELEMENTS (gdm_session_worker_error_entries) - 1 > == GDM_SESSION_WORKER_ERROR_IN_REAUTH_SESSION); > > What's the point of this assert? I stole it from the GDBusError docs. http://developer.gnome.org/gio/unstable/gio-GDBusError.html#gio-GDBusError.description > It doesn't actuallly protect against anything > does it? It would be useful to try to catch the enum growing without the > mapping getting updated, but this doesn't achieve that. Why not? > G_STATIC_ASSERT (GDM_SESSION_WORKER_NUMBER_OF_ERRORS - 1 == > GDM_SESSION_WORKER_ERROR_IN_REAUTH_SESSION); This doesn't care about the mapping at all. > ::: daemon/gdm-session-worker.c > @@ +2093,1 @@ > g_error_free (error); > > you could combine these two lines into g_dbus_method_invocation_take_error if > you wanted Thanks, didn't know about that. > ::: daemon/gdm-session.c > @@ +779,3 @@ > + } else { > + report_problem_and_stop_conversation (self, service_name, > error->message); > + gdm_session_stop_conversation (self, service_name); > > This gdm_session_stop_conversation is redundant I think after > report_problem_and_stop_conversation Whoops.
Hi, (In reply to comment #94) > (In reply to comment #93) > > Review of attachment 220031 [details] [review] [details]: > > > > seems fine, nice clean up overall. A few comments below, but otherwise ready to > > commit I think. > > > > ::: daemon/gdm-session-worker-common.c > > @@ +53,3 @@ > > + gdm_session_worker_error_entries, > > + G_N_ELEMENTS > > (gdm_session_worker_error_entries)); > > + G_STATIC_ASSERT (G_N_ELEMENTS (gdm_session_worker_error_entries) - 1 > > == GDM_SESSION_WORKER_ERROR_IN_REAUTH_SESSION); > > > > What's the point of this assert? > > I stole it from the GDBusError docs. > > http://developer.gnome.org/gio/unstable/gio-GDBusError.html#gio-GDBusError.description Hmm, so either I'm missing something or we should get the docs fixed, too. At a minimum, the G_STATIC_ASSERT should be at the top of the function before any lines of code are executed. > > It doesn't actuallly protect against anything > > does it? It would be useful to try to catch the enum growing without the > > mapping getting updated, but this doesn't achieve that. > > Why not? Because there's nothing to enforcing that you full use the enum. If you have, enum { ZERO, ONE, TWO, THREE }; and then do static struct { int value; char *name } number_mapping[] = { { ZERO, "zero" }, { { ONE, "one" } }; then G_N_ELEMENTS (number_mapping) will be 2 (the number of things you mapped), and G_N_ELEMENTS (number_mapping) - 1 == ONE } will be true (so the last thing you mapped is the last thing in the map), and yet 2 items won't get mapped. > > G_STATIC_ASSERT (GDM_SESSION_WORKER_NUMBER_OF_ERRORS - 1 == > > GDM_SESSION_WORKER_ERROR_IN_REAUTH_SESSION); > > This doesn't care about the mapping at all. But GDM_SESSION_WORKER_NUMBER_OF_ERRORS value will be automatically incremented when the enum grows a new item, so NUMBER_OF_ERRORS - 1 will no longer equal IN_REAUTH_SESSION when someone changes the enum and then the programmer who added the entry will see the compile error, see why it failed, and fix the mapping and the assert. If you don't think that's a strong enough test, then you could have a for (int i = 0; i < NUMBER_OF_ERRORS; i++) { switch (error_entries[i]) { case ERROR_GENERIC ... ERROR_IN_REAUTH_SESSION: break; } } and then let the compiler warn if the case range operands didn't cover the entire enum. I personally think that's way overboard,think G_STATIC_ASSERT (NUMBER_OF_ERRORS - 1 == IN_REAUTH_SESSION) is also overboard, and think G_STATIC_ASSERT (G_N_ELEMENTS (error_entries) - 1 == IN_REAUTH_SESSION) doesn't help anything. So unless I'm missing something, I still think you should just drop the line and also think we should get the docs fixed. That's just my opinion-you're the one investing time in this patch, if you feel strongly about having some sort of assert here feel free.
Attachment 220025 [details] pushed as 67235fd - daemon: Provide mechanism for providing an authentication secret up front Attachment 220026 [details] pushed as 2ebfa91 - daemon: Rename GdmWelcomeSession to GdmLaunchEnvironment Attachment 220027 [details] pushed as 4e2a75a - daemon: Add initial setup integration Attachment 220031 [details] pushed as 2672f6a - daemon: Replace old method/signal-based API with async method calls Pushed outstanding patches with suggested changes. Dropped assert.
Filed docs bug as https://bugzilla.gnome.org/show_bug.cgi?id=680994
Review of attachment 220025 [details] [review]: Did someone test this? Looking at the source code of libpam, it is clear that: case PAM_AUTHTOK: /* * PAM_AUTHTOK and PAM_OLDAUTHTOK are only accessible from * modules. */ if (__PAM_FROM_MODULE(pamh)) { if (pamh->authtok != item) { _pam_overwrite(pamh->authtok); TRY_SET(pamh->authtok, item); } } else { retval = PAM_BAD_ITEM; } break; And therefore this does not work at all. (I found this while implementing the pam helper for accountsservice to change passwords. Source code is from Linux-PAM 1.1.5)
well, poo.
Now what?
So we have a few options I can think of, I guess: 1) try to hack around the check (bleh) 2) ship our own pam module to do the pam_set_item for us 3) feed the password to the first echo off query that comes out of the conversation.
all three options sort of suck
If we go with 3, i'd say we should do it at the gnome-initial-setup level
pam sort of sucks I don't know what 1) would imply, other than tricking PAM into thinking that the worker is a module. I'm quite sure that would be a big security issue if found.
by 1) i meant something gross like struct shadow_of_pam_handle { whatever struct pam_handle has in it here }; ((shadow_of_pam_handle *) pamh)->authtok = strdup (saved_password);
Since initial setup is a "direct after install" scenario, I think we can guarantee the PAM stack here. Just responding to the first SecretInfoQuery would be fine would me.
Hi, Just testing gdm 3.5.5 and it seems like 2672f6a1f43d054c0aab34b623f0f0e6edd8ed5f (daemon: Replace old method/signal-based API with async method calls) is a bad commit for me (although I've also reverted 67235fd797e5b9a88178f4733551814b61a4711b (daemon: Provide mechanism for providing an authentication secret up front) as it was also giving me some (red herring) errors in the logs). The problem manifests itself when unlocking the session. It initially looks like it's unlocked fine, but then almost immediately logs me out. Checcking the locks I see that GDM has crashed in worker_on_reauthenticated(). A quick git blame and some reverts and I confirmed the above commit. I have now locked and unlocked quite happily after rebooting. I'll attach the backtrace momentarily.
Created attachment 220971 [details] Some log and a backtrace of fault introduced in 3.5.5 The log shows the invalid signal that the second commit mentioned in the above comment. It doesn't seem to be fatal, but I guess it could be somewhat related (i.e. the crash is triggered due to the fact that that signal is not handled as it should be), but either way gdm shouldn't crash. I'm also not 100% sure but I think before reverting the problem commit, that if I got the password wrong on the login screen it would give me an ugly dbus/gobject error rather than the proper "Authentication Failed" toaster popup.
Created attachment 221019 [details] [review] GdmSessionWorker: fix signature of Reauthenticated signal GdmSession expects to receive the service name along with the signal, but the DBus interface does not include it, and this causes a crash. ------ Found the problem: there is a mismatch in the DBus signature (and generated GObject signal) and the GdmSession handler, which causes the latter to access garbage memory and segfault.
Comment on attachment 221019 [details] [review] GdmSessionWorker: fix signature of Reauthenticated signal looks good
Review of attachment 221019 [details] [review]: We already have the service name as part of the conversation struct. Just get it from that instead of DBus.
Created attachment 221026 [details] [review] GdmSession: don't access conversation after closing the connection When the connection is closed, the conversation is freed, but method calls that are in flight would see their async callbacks invoked at the next mainloop iteration, with the error set. Return early in that case.
Review of attachment 221026 [details] [review]: Huh, OK.
Comment on attachment 221026 [details] [review] GdmSession: don't access conversation after closing the connection Attachment 221026 [details] pushed as f073821 - GdmSession: don't access conversation after closing the connection
Created attachment 221027 [details] [review] GdmSession: fix signature of Reauthenticated signal handler GdmSession expects to receive the service name along with the signal, but the DBus interface does not include it, and this causes a crash. This is the other way round.
Review of attachment 221027 [details] [review]: Much better. My goal with these cleanups was to remove service_name from the DBus interface entirely, since it's unnecessary.
You didn't complete that, though. Well, bah... Attachment 221027 [details] pushed as 27d14a6 - GdmSession: fix signature of Reauthenticated signal handler
I think you guys are conflating the service name that was used to log into the session with the service name that was used to unlock the session. I think the first patch was right (though I'd have to do more digging to be sure)
Uh oh. Could be. Well, anyway that service_name is bubbled up to GdmSimpleSlave and then just ignored...
still, you could imagine a the screensaver wanting know if the user unlocked with fingerprint or password so it's good to be accurate.
Created attachment 221667 [details] [review] daemon: Fix session setting code Passing ['--session=', 'foo'] won't be picked up by GOption. Pass [--session', 'foo'] instead.
Created attachment 221668 [details] [review] Revert "daemon: Provide mechanism for providing an authentication secret up front" This reverts commit 67235fd797e5b9a88178f4733551814b61a4711b. As pointed out by Giovanni, this code is incorrect, as PAM_AUTHTOK doesn't work when not in a PAM module.
Attachment 221667 [details] pushed as e476672 - daemon: Fix session setting code Attachment 221668 [details] pushed as ff7bfd2 - Revert "daemon: Provide mechanism for providing an authentication secret up front"
(reopening since initial-setup work is still pending and may need more gdm changes)
Created attachment 225909 [details] [review] Enable initial setup by default
all done here now, I think