GNOME Bugzilla – Bug 677548
Add 'Enterprise Login' via user panel
Last modified: 2012-06-12 19:09:02 UTC
To support Active Directory (and other types of realm logins) the add user account should be able to add 'Enterprise logins'. The user types the domain, their login name and password. The machine detects the type of domain/realm. The machine is joined to the domain if necessary, and their login is configured for login on the local machine. This bug is about the parts in the add dialog. The user panel also needs changes when dealing with non-local users. That will be done separately. You can see updated add account in one of the design mockups here. Well almost, the Google Account stuff isn't relevant: https://live.gnome.org/Design/Proposals/UserIdentities https://live.gnome.org/Design/Proposals/UserIdentities?action=AttachFile&do=get&target=user-accounts-add.png
Implementation is here: http://git.gnome.org/browse/gnome-control-center/log/?h=wip/add-account This depends on: * AccountsService.CacheUser(): https://bugs.freedesktop.org/show_bug.cgi?id=50770 * Runtime dependency on realmd: http://git.freedesktop.org/~stefw/realmd I'll be working on getting the above dependencies squared away... But until then, since this is my first big contribution to control center, it'd be awesome to get an initial review as a sanity check.
(In reply to comment #1) > Implementation is here: > http://git.gnome.org/browse/gnome-control-center/log/?h=wip/add-account Attach it as one of more logically separate patches, for review.
Created attachment 215795 [details] [review] user-accounts: Remove unused MAXNAMELEN definition
Created attachment 215796 [details] [review] user-accounts: Make UmAccountDialog a GtkDialog The dialog will get much more complex due to 'enterprise login' work and we want a proper object here.
Created attachment 215797 [details] [review] user-accounts: Don't refer to UmAccountDialog as um It's confusing to read; it reads as 'user manager'. Since this is a GObject, just use self as customary. Most of these lines change in later commits anyway, so this won't really polute the 'git blame'
Created attachment 215798 [details] [review] user-accounts: Cleaner interaction with UmAccountDialog actions More clear scoping and interaction with running actions in UmAccountDialog. In later 'enterprise login' patches we have long running actions that's why this needs cleaning up. In particular: * Show errors as children of the dialog. * Errors don't make the account dialog go away, user can correct problems. * Use more standard GAsyncResult style callbacks: um_account_dialog_perform() um_account_dialog_finish() * Disable controls while the operation is happening.
Created attachment 215799 [details] [review] user-accounts: User to cancel long actions in UmAccountDialog Allow the user to cancel long actions in UmAccountDialog by pressing the cancel button. This is relevant in later commits when we merge 'enterprise login' support.
Created attachment 215800 [details] [review] user-accounts: Show a spinner during account dialog actions During actions which can run a long time show a spinner indicating that something is going on.
Created attachment 215801 [details] [review] user-accounts: Add test tool frob-account-dialog A simple tool to show the add account dialog for quick iterations testing of the 'enterprise login' functionality.
Created attachment 215802 [details] [review] user-accounts: Title of accounts dialog becomes "Add Account" This title makes sense for both remote and local users. In the case of remote users we're not creating accounts, we're just adding them to the system.
Created attachment 215803 [details] [review] user-accounts: Port account dialog to GtkGrid Some spacing issues will be sorted in a later commit.
Created attachment 215804 [details] [review] user-accounts: Fix memory leak of GtkBuilder in accounts dialog
Created attachment 215805 [details] [review] user-accounts: Separate local account stuff in accounts dialog Separate the local account stuff a bit so when the enterprise stuff comes in it's still readable.
Created attachment 215806 [details] [review] user-accounts: Cleaner validation and validate on open Clean up the validation so we can plug in the enterprise login stuff. Also fix a bug where the dialog was not validated when it was initially opened.
Created attachment 215807 [details] [review] user-accounts: Add switcher between modes Add the switcher between the local account and enterprise modes. The enterprise area just has a place holder widget for now.
Created attachment 215808 [details] [review] user-accounts: Add um_user_manager_cache_user() This calls the AccountsService.CacheUser() method to register a user that's not in /etc/passwd Depends on: https://bugs.freedesktop.org/show_bug.cgi?id=50770
Created attachment 215809 [details] [review] user-accounts: Implement enterprise logins in add dialog * Use realmd for domain joining and lookup, runtime dependency * Validate join domain correctly * Add UmRealmManager for handling some stuff above the autogenerated realmd dbus code * Show a dialog if the user's credentials cannot be used to join the domain. Prompt for admin creds. * Register the user's login with the AccountsService * This depends on the CacheUser() method of AccountsService
There you go. I've split it up, as requested.
Review of attachment 215795 [details] [review]: Interesting...maybe we should use it ? Anyway, we can easily bring it back when somebody gets bitten by too long names...
Review of attachment 215796 [details] [review]: Looks fine to me.
Review of attachment 215797 [details] [review]: Might want to squish this with the previous commit that turned it into a class ?
Review of attachment 215796 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ +51,3 @@ +typedef struct { + GtkDialogClass parent_class; +} UmAccountDialogClass; Shouldn't that be in the .h file? @@ +243,3 @@ + break; + default: + g_warn_if_reached (); Pretty certain you don't want to do that if you don't control the function that adds buttons to the dialogue.
Review of attachment 215797 [details] [review]: I prefer those commits separate, so fine by me.
Review of attachment 215798 [details] [review]: If you have long running actions, we probably need to be able to cancel those actions too. ::: panels/user-accounts/um-account-dialog.h @@ -41,2 @@ GtkWindow *parent, - UserCreatedCallback user_created, Remove the definition of the UserCreatedCallback func too? ::: panels/user-accounts/um-user-panel.c @@ +342,3 @@ add_user (GtkButton *button, UmUserPanelPrivate *d) { + um_account_dialog_perform (d->account_dialog, Not sure I'm happy with "perform"
Review of attachment 215799 [details] [review]: Could we merge this with the previous patch?
Review of attachment 215800 [details] [review]: Looks good
Review of attachment 215801 [details] [review]: Useful.
Review of attachment 215802 [details] [review]: Fine.
Review of attachment 215803 [details] [review]: Looks good.
Review of attachment 215804 [details] [review]: This should be merged into a prior patch, or done earlier (this even applies to gnome-3-4).
Review of attachment 215805 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ +43,3 @@ GtkSpinner *spinner; + /* Local widgets */ Local user account widgets @@ +159,3 @@ account_type, self->cancellable, + (GAsyncReadyCallback)on_manager_create_user, This looks like a gratuitous name change
Review of attachment 215806 [details] [review]: Can you please separate the 2 functional parts of this patch? The validation parts likely needs to be added in gnome3-4.
Review of attachment 215807 [details] [review]: Stuff to switch between things needs some work... ::: panels/user-accounts/um-account-dialog.c @@ +51,3 @@ GtkSpinner *spinner; + /* Bar switcher stuff */ What's a bar switcher stuff? @@ +540,2 @@ local_prepare (self); + switcher_switch (self, UM_LOCAL); That's some lousy naming there :)
Review of attachment 215808 [details] [review]: ::: panels/user-accounts/um-user-manager.c @@ +466,3 @@ } + else if (g_dbus_error_is_remote_error (error) && + strcmp (g_dbus_error_get_remote_error(error), "org.freedesktop.Accounts.Error.UserDoesNotExist") == 0) { g_dbus_error_get_remote_error() returns an allocated string.
Review of attachment 215809 [details] [review]: ::: configure.ac @@ +227,3 @@ +# Kerberos kerberos support +AC_PATH_PROG(KRB5_CONFIG, krb5-config) No automagic please. Either krb5-config is there, or we error out. @@ +233,3 @@ + KRB5_LIBS="`$KRB5_CONFIG --libs`" + AC_MSG_RESULT(yes) +elif test x$KRB5_PASSED_LIBS = x; then Where is KRB5_PASSED_LIBS defined? ::: panels/user-accounts/Makefile.am @@ +24,3 @@ endif +BUILT_SOURCES = \ Does this need to be added to CLEANFILES too? ::: panels/user-accounts/data/org.freedesktop.realmd.xml @@ +4,3 @@ +<node name="/"> + <!-- + * Global interface implemented by realmd. Allows listing of providers Could we not have this file provided by realmd itself, with a versioned pkg-config file? ::: panels/user-accounts/um-realm-manager.c @@ +154,3 @@ + + g_variant_iter_init (&iter, realms); + for (;;) { I don't like empty for loops. Use while() or really use for properly please. ::: panels/user-accounts/um-realm-manager.h @@ +66,3 @@ +typedef enum { + UM_REALM_LOGIN_TO_CREDENTIALS = 1 << 0, +} UmRealmLoginFlags; If you don't foresee other flags being used straight away, I'd rather they weren't added.
Review of attachment 215795 [details] [review]: Committed.
Review of attachment 215796 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ +51,3 @@ +typedef struct { + GtkDialogClass parent_class; +} UmAccountDialogClass; UmAccountDialog is a 'final' class that cannot be derived from. Thus the object and class structs are in the .c file. Moved the typedef to the .h file in any case. @@ +243,3 @@ + break; + default: + case GTK_RESPONSE_OK: Removed.
Review of attachment 215804 [details] [review]: Committed earlier, and back ported to gnome-3-4. I tried my best to test the back port, but gave up when I discovered g-c-c gnome-3-4 no longer builds against goa in jhbuild. But I can't see how this would break things.
Review of attachment 215799 [details] [review]: Merged into previous.
Review of attachment 215798 [details] [review]: ::: panels/user-accounts/um-account-dialog.h @@ -41,2 @@ GtkWindow *parent, - UserCreatedCallback user_created, Yes, it's no longer necessary as we use the standard GAsyncReadyCallback + finish func. ::: panels/user-accounts/um-user-panel.c @@ +342,3 @@ add_user (GtkButton *button, UmUserPanelPrivate *d) { + um_account_dialog_perform (d->account_dialog, Ok. Changed back to 'show'.
Created attachment 216173 [details] [review] user-accounts: Cleaner interaction with UmAccountDialog actions More clear scoping and interaction with running actions in UmAccountDialog. In later 'enterprise login' patches we have long running actions that's why this needs cleaning up. In particular: * Show errors as children of the dialog. * Errors don't make the account dialog go away, user can correct problems. * Use more standard GAsyncResult style callbacks: um_account_dialog_perform() um_account_dialog_finish() * Disable controls while the operation is happening. * Allow the user to cancel long actions in UmAccountDialog by pressing the cancel button.
Created attachment 216174 [details] [review] user-accounts: Add test tool frob-account-dialog A simple tool to show the add account dialog for quick iterations testing of the 'enterprise login' functionality.
Review of attachment 216174 [details] [review]: Bring over status
Review of attachment 215805 [details] [review]: Updated patch.
Created attachment 216175 [details] [review] user-accounts: Separate local account stuff in accounts dialog Separate the local account stuff a bit so when the enterprise stuff comes in it's still readable.
Review of attachment 215806 [details] [review]: Filed a separate bug for validation stuff, and added dependency.
Review of attachment 215807 [details] [review]: Fixed.
Review of attachment 215808 [details] [review]: ::: panels/user-accounts/um-user-manager.c @@ +466,3 @@ } + else if (g_dbus_error_is_remote_error (error) && + strcmp (g_dbus_error_get_remote_error(error), "org.freedesktop.Accounts.Error.UserDoesNotExist") == 0) { Factored this out into a different bug, since the problem exists in surrounding code as well.
Review of attachment 216175 [details] [review]: Looks good.
Review of attachment 216173 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ +38,3 @@ struct _UmAccountDialog { GtkDialog parent; + GtkWidget *widgets; Need to find a better name than widgets for a single widget. content_widget? container_widget?
Review of attachment 216173 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ +38,3 @@ struct _UmAccountDialog { GtkDialog parent; + GtkWidget *widgets; Changed to container_widget.
Created attachment 216184 [details] [review] user-accounts: Cleaner interaction with UmAccountDialog actions More clear scoping and interaction with running actions in UmAccountDialog. In later 'enterprise login' patches we have long running actions that's why this needs cleaning up. In particular: * Show errors as children of the dialog. * Errors don't make the account dialog go away, user can correct problems. * Use more standard GAsyncResult style callbacks: um_account_dialog_perform() um_account_dialog_finish() * Disable controls while the operation is happening. * Allow the user to cancel long actions in UmAccountDialog by pressing the cancel button.
Created attachment 216185 [details] [review] user-accounts: Separate local account stuff in accounts dialog Separate the local account stuff a bit so when the enterprise stuff comes in it's still readable.
Created attachment 216186 [details] [review] user-accounts: Cleaner validation and validate on open Clean up the validation so we can plug in the enterprise login stuff.
Created attachment 216187 [details] [review] user-accounts: Add switcher between modes Add the switcher between the local account and enterprise modes. The enterprise area just has a place holder widget for now.
Created attachment 216188 [details] [review] user-accounts: Add um_user_manager_cache_user() This calls the AccountsService.CacheUser() method to register a user that's not in /etc/passwd Depends on: https://bugs.freedesktop.org/show_bug.cgi?id=50770
Created attachment 216189 [details] [review] user-accounts: Implement enterprise logins in add dialog * Use realmd for domain joining and lookup, runtime dependency * Validate join domain correctly * Add UmRealmManager for handling some stuff above the autogenerated realmd dbus code * Show a dialog if the user's credentials cannot be used to join the domain. Prompt for admin creds. * Register the user's login with the AccountsService * This depends on the CacheUser() method of AccountsService
Review of attachment 216187 [details] [review]: ::: panels/user-accounts/um-account-dialog.c @@ +298,3 @@ + +static void +switcher_switch_mode (UmAccountDialog *self, switcher change mode. Or better set_switch_mode. @@ +336,3 @@ + +static void +switcher_toggle (UmAccountDialog *self, toggle_switch() @@ +445,2 @@ local_area_init (self, builder); + switcher_init (self, builder); It really isn't a switcher, it's a switch. The switcher is the guy moving the switch.
Review of attachment 216188 [details] [review]: ::: panels/user-accounts/um-user-manager.c @@ +467,3 @@ data->user_name); } + else if (g_dbus_error_is_remote_error (error) && On the same line as the preceding curly brace please. @@ +625,3 @@ "Not authorized"); } + else if (g_dbus_error_is_remote_error (error) && On the same line as the preceding curly brace please.
Created attachment 216190 [details] [review] user-accounts: Add way to change modes Add the buttons to switch between the local account and enterprise modes. The enterprise area just has a place holder widget for now.
Created attachment 216191 [details] [review] user-accounts: Add um_user_manager_cache_user() This calls the AccountsService.CacheUser() method to register a user that's not in /etc/passwd Depends on: https://bugs.freedesktop.org/show_bug.cgi?id=50770
Review of attachment 216189 [details] [review]: ::: panels/user-accounts/um-realm-manager.c @@ +756,3 @@ + login = g_simple_async_result_get_op_res_gpointer (async); + if (credentials && login->credentials) + *credentials = g_bytes_ref (login->credentials); You're not setting *credentials if login->credentials doesn't exist. You should set it to NULL. ::: panels/user-accounts/um-realm-manager.h @@ +30,3 @@ + UM_REALM_ERROR_BAD_LOGIN, + UM_REALM_ERROR_BAD_PASSWORD, + UM_REALM_ERROR_FAIL, ERROR_GENERIC? Or does FAIL mean something else?
Created attachment 216192 [details] [review] user-accounts: Implement enterprise logins in add dialog * Use realmd for domain joining and lookup, runtime dependency * Validate join domain correctly * Add UmRealmManager for handling some stuff above the autogenerated realmd dbus code * Show a dialog if the user's credentials cannot be used to join the domain. Prompt for admin creds. * Register the user's login with the AccountsService * This depends on the CacheUser() method of AccountsService
Created attachment 216193 [details] [review] user-accounts: Implement enterprise logins in add dialog * Use realmd for domain joining and lookup, runtime dependency * Validate join domain correctly * Add UmRealmManager for handling some stuff above the autogenerated realmd dbus code * Show a dialog if the user's credentials cannot be used to join the domain. Prompt for admin creds. * Register the user's login with the AccountsService * This depends on the CacheUser() method of AccountsService
Review of attachment 216189 [details] [review]: Done.