Created attachment 81645 [details] [review]
ActUserManager: notify user object as loaded in case of failure

Won't this just make user objects for non-existing users appear on the client-side ? How will gdm know that this user does not exist ?

At the moment libaccountsservice has no way to tell the developer that a given username doesn't exist on the system. Mostly because of the way the library is designed. The ActUserManager allows to create ActUser objects without any communication with the daemon, and loads their data asynchronously. So you can already create user objects for user that do not exist.

The main difference between existing/non-existing users is that the object gets loaded when it exists. If not, then the developer is never notified that the user doesn't exist.

Back to the Gdm case, what happens is that Gdm timeouts without displaying anything, because the user's data can never be loaded. With this patch the ActUser object now reports that the data is loaded, even though it's not valid data, but it doesn't really matter because Gdm is going to go ahead and try to login, and it will fail at the next step when we actually try to authenticate.

I agree this isn't a perfect solution. I could like to have a property on the ActUser object saying it's invalid. But that requires API changes/additions.

For clarity, in the GDM source code we have this:

        /* Load settings from accounts daemon before continuing
         * FIXME: need to handle username not loading
         */
        worker->priv->pending_invocation = invocation;
        if (gdm_session_settings_load (worker->priv->user_settings, username)) {
                queue_state_change (worker);                          
        } else {
                g_signal_connect (G_OBJECT (worker->priv->user_settings),
                                  "notify::is-loaded",
                                  G_CALLBACK (on_settings_is_loaded_changed),
                                  worker);  
        }

That pending_invocation never gets returned because is-loaded never gets set

Hey thanks for the patch!

I've pushed a variation of this patch that let's callers test for the nonexistence of users:

http://cgit.freedesktop.org/accountsservice/commit/?id=254e959f71c44c95c932f9dd04ad809c32aaee4e

Cool, you will need to modify Gdm too.

I don't think that's absolutely required.  is-loaded is getting set to true just as before.  The updates just ensure that a caller can check for nonexistence if it wants to, and we don't really need it for GDM.  The only reason it's asking for a user account at all is to potentially load the user's saved session, which isn't necessary if the user doesn't exist.

Sorry, missed the set_is_loaded() call.

Shouldn't ActUser just be GInitable? That way it can return an error when an ActUser is created for a non-existant user?

it could be GAsyncInitable , but we would need to break the api.

If we're going to break the api, I'd rather make the library as thin a wrapper around dbus codegen as possible, and if necesary, use GTask for async operations not coverable strictly by codegen.

Fair enough.