Bug 66325

Summary: user is never set loaded if it doesn't exist
Product: accountsservice Reporter: Lionel Landwerlin <lionel.g.landwerlin>
Component: generalAssignee: Matthias Clasen <mclasen>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: marius.vollmer, rstrode, stefw
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: ActUserManager: notify user object as loaded in case of failure

Description Lionel Landwerlin 2013-06-28 14:19:49 UTC
Gdm has a feature allowing you to login as a user not listed on the screen.
Unfortunately this feature will just timeout if you enter a username that doesn't exist on the system.
That happens because libaccountsservice doesn't set the user as loaded when it receives a DBus response from the daemon saying that the user doesn't exist.
Comment 1 Lionel Landwerlin 2013-06-28 14:20:21 UTC
Created attachment 81645 [details] [review]
ActUserManager: notify user object as loaded in case of failure
Comment 2 Matthias Clasen 2013-06-30 01:47:35 UTC
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 ?
Comment 3 Lionel Landwerlin 2013-06-30 10:27:26 UTC
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.
Comment 4 Ray Strode [halfline] 2013-07-01 13:07:30 UTC
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
Comment 5 Ray Strode [halfline] 2013-07-01 13:52:28 UTC
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
Comment 6 Lionel Landwerlin 2013-07-01 13:54:24 UTC
Cool, you will need to modify Gdm too.
Comment 7 Ray Strode [halfline] 2013-07-01 14:14:30 UTC
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.
Comment 8 Lionel Landwerlin 2013-07-01 14:23:14 UTC
Sorry, missed the set_is_loaded() call.
Comment 9 Stef Walter 2013-07-01 16:26:29 UTC
Shouldn't ActUser just be GInitable? That way it can return an error when an ActUser is created for a non-existant user?
Comment 10 Ray Strode [halfline] 2013-07-01 16:38:27 UTC
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.
Comment 11 Stef Walter 2013-07-01 16:39:18 UTC
Fair enough.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.