After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 727871 - g-c-c using userdel for users in AD (I'm using SSSD)
g-c-c using userdel for users in AD (I'm using SSSD)
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: User Accounts
3.10.x
Other Linux
: Normal major
: ---
Assigned To: Ondrej Holy
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-04-09 05:32 UTC by Igor Gnatenko
Modified: 2014-11-03 17:18 UTC
See Also:
GNOME target: ---
GNOME version: 3.9/3.10


Attachments
user list (41.43 KB, image/png)
2014-04-09 05:32 UTC, Igor Gnatenko
  Details
del user (18.06 KB, image/png)
2014-04-09 05:32 UTC, Igor Gnatenko
  Details
bug (10.02 KB, image/png)
2014-04-09 05:33 UTC, Igor Gnatenko
  Details
user-accounts: allow removing only local accounts (1.21 KB, patch)
2014-09-18 14:02 UTC, Ondrej Holy
none Details | Review
fix enterprise accounts deleting (14.32 KB, patch)
2014-09-22 16:54 UTC, Ondrej Holy
reviewed Details | Review
user-accounts: use common function for error messages (3.14 KB, patch)
2014-09-30 09:19 UTC, Ondrej Holy
committed Details | Review
user-accounts: fix enterprise accounts deleting (11.37 KB, patch)
2014-09-30 09:20 UTC, Ondrej Holy
committed Details | Review

Description Igor Gnatenko 2014-04-09 05:32:08 UTC
Steps to reproduce:
1. realm join
2. login in gdm with AD account
3. logout
4. login with local user
5. open gcc and try to delete AD user

Actual results:
userdel failed

Expected results:
user deleted with SSSD clean cache or something like this (I don't know)

Additional info:
see screens
Comment 1 Igor Gnatenko 2014-04-09 05:32:28 UTC
Created attachment 273848 [details]
user list
Comment 2 Igor Gnatenko 2014-04-09 05:32:49 UTC
Created attachment 273849 [details]
del user
Comment 3 Igor Gnatenko 2014-04-09 05:33:09 UTC
Created attachment 273850 [details]
bug
Comment 4 Ondrej Holy 2014-09-18 14:02:45 UTC
Created attachment 286502 [details] [review]
user-accounts: allow removing only local accounts

There are some downstream bug reports about:
https://bugzilla.redhat.com/show_bug.cgi?id=953829
https://bugs.launchpad.net/ubuntu/+source/accountsservice/+bug/988072

It would be best to allow removing users only for local accounts until it will be fixed...
Comment 5 Ondrej Holy 2014-09-18 14:33:57 UTC
There is another downstream bug report with info how to fix it:
https://bugzilla.redhat.com/show_bug.cgi?id=1060183#c6
Comment 6 Matthias Clasen 2014-09-18 16:56:06 UTC
Given that we only cache remote users (not really create them), it would seem logical to uncache them (instead of deleting them)
Comment 7 Ondrej Holy 2014-09-22 16:54:52 UTC
Created attachment 286826 [details] [review]
fix enterprise accounts deleting

Uncache enterprise users from the acountsservice and consequently deny them from realm service when deleting enterprise accounts. This way we don't have control about user's data. I think accountsservice or realmd should take care about, but there isn't api for it...
Comment 8 Ondrej Holy 2014-09-22 16:56:27 UTC
This change for accountsservice is needed for:
https://bugs.freedesktop.org/show_bug.cgi?id=84091
Comment 9 Ray Strode [halfline] 2014-09-29 20:32:11 UTC
Review of attachment 286826 [details] [review]:

hey thanks for looking at this.

::: panels/user-accounts/um-user-panel.c
@@ +114,3 @@
+{
+        g_object_unref (data->self);
+        g_free (data->login);

don't you need to free the data slice too?

@@ +120,3 @@
+show_error_dialog (CcUserPanelPrivate *d,
+                   const gchar *message,
+                   GError *error)

this function seems like an orthogonal clean up and should be a separate (pre-requisite) patch

@@ +501,3 @@
+        GError *error = NULL;
+
+        if (g_cancellable_is_cancelled (d->cancellable)) {

i was going to suggest just checking the error code for CANCELLED on the finish call, but what you're doing follows an idiom already established in the code (which probably predates set_check_cancellable and g_task) so I think it's probably fine.

@@ +520,3 @@
+        UmRealmCommon *common = NULL;
+        GList *realms, *l;
+        const gchar * const*permitted_logins;

spaces around asterisk. move declaration down to inside the loop.

@@ +521,3 @@
+        GList *realms, *l;
+        const gchar * const*permitted_logins;
+        gint i;

this too.

@@ +536,3 @@
+
+                g_object_unref (common);
+                common = NULL;

g_clear_object

@@ +538,3 @@
+                common = NULL;
+        }
+        g_list_free (realms);

this needs to be g_list_free_full right?

@@ +574,3 @@
+        if (common == NULL) {
+                /* The realm was probably leaved */
+                g_warning ("Unable to find matching realm");

i don't think you want a g_warning here. should be non-exceptional for admin to leave a realm. also s/leaved/left/

@@ +580,3 @@
+
+        /* Remove the user from permitted logins */
+        g_debug ("Denying login for: %s", data->login);

s/login/future logins/

@@ +590,3 @@
+                                                  add, remove, options,
+                                                  d->cancellable,
+                                                  enterprise_user_denied,

maybe "revoked" instead of "denied" ?

@@ +617,3 @@
+        }
+        else {
+                show_error_dialog (d, _("Failed to delete user"), error);

"Failed to revoke remotely managed user" maybe.  we're not actually deleting the user after all.

@@ +1794,3 @@
+        CcUserPanel *self = UM_USER_PANEL (object);
+
+        g_clear_object (&self->priv->cancellable);

you already clear this in dispose, why are you adding a finalize handler?
Comment 10 Ondrej Holy 2014-09-30 09:18:55 UTC
(In reply to comment #9)
> Review of attachment 286826 [details] [review]:
> 
> hey thanks for looking at this.

Thanks for the review! I am slightly ashamed the review is too long...
 
> ::: panels/user-accounts/um-user-panel.c
> @@ +501,3 @@
> +        GError *error = NULL;
> +
> +        if (g_cancellable_is_cancelled (d->cancellable)) {
> 
> i was going to suggest just checking the error code for CANCELLED on the finish
> call, but what you're doing follows an idiom already established in the code
> (which probably predates set_check_cancellable and g_task) so I think it's
> probably fine.

I do it this way, because it is used similarly in other files. It is there to avoid segfaults when user goes back to g-c-c overview and delete procedure isn't finished yet. 

If cancellable is cancelled, show_error_dialog is executed and it fails, because dialog is already disposed... or do you have better idea how to do it?

> @@ +590,3 @@
> +                                                  add, remove, options,
> +                                                  d->cancellable,
> +                                                  enterprise_user_denied,
> 
> maybe "revoked" instead of "denied" ?

I use "denied" to correspond with realm commandline option "realm deny", but we can use "revoked"... 
 
> @@ +617,3 @@
> +        }
> +        else {
> +                show_error_dialog (d, _("Failed to delete user"), error);
> 
> "Failed to revoke remotely managed user" maybe.  we're not actually deleting
> the user after all.

We aren't deleting the user, but from users point of view it is deleting, but I will fix it...

> @@ +1794,3 @@
> +        CcUserPanel *self = UM_USER_PANEL (object);
> +
> +        g_clear_object (&self->priv->cancellable);
> 
> you already clear this in dispose, why are you adding a finalize handler?

I don't clear it in dispose, there is only:
g_cancellable_cancel (priv->cancellable);

but I'll do this and store reference to cancellable in AsyncDeleteData instead...
Comment 11 Ondrej Holy 2014-09-30 09:19:52 UTC
Created attachment 287432 [details] [review]
user-accounts: use common function for error messages
Comment 12 Ondrej Holy 2014-09-30 09:20:36 UTC
Created attachment 287433 [details] [review]
user-accounts: fix enterprise accounts deleting
Comment 13 Ondrej Holy 2014-09-30 09:25:04 UTC
(In reply to comment #9)
> Review of attachment 286826 [details] [review]:
> @@ +617,3 @@
> +        }
> +        else {
> +                show_error_dialog (d, _("Failed to delete user"), error);
> 
> "Failed to revoke remotely managed user" maybe.  we're not actually deleting
> the user after all.

So maybe we should to change also this string:
"Are you sure you want to permanently delete %s's account?"
Comment 14 Ray Strode [halfline] 2014-09-30 13:31:02 UTC
Review of attachment 287432 [details] [review]:

+
Comment 15 Ray Strode [halfline] 2014-09-30 13:44:59 UTC
Review of attachment 287433 [details] [review]:

thanks for the quick turn around. looks pretty good to me. it adds new strings, so you probably want to branch before pushing.

::: panels/user-accounts/um-user-panel.c
@@ +547,3 @@
+
+static void
+realm_manager_found (GObject *source,

might rename this to realm_manager_found_to_remove_user or something like that to make it clear the purpose of the function, but not doesn't really matter.
Comment 16 Ray Strode [halfline] 2014-09-30 13:45:33 UTC
Hi,
> So maybe we should to change also this string:
> "Are you sure you want to permanently delete %s's account?"
yea it would be a good idea.
Comment 17 Ondrej Holy 2014-09-30 14:08:08 UTC
Thanks for the review, do you think "Are you sure you want to revoke remotely managed %s's account?" is better (and correct)?
Comment 18 Ondrej Holy 2014-11-03 17:14:11 UTC
Comment on attachment 287432 [details] [review]
user-accounts: use common function for error messages

commit ed13a62c1d17105bd40c110ddbfed2b15cfa1b48
Comment 19 Ondrej Holy 2014-11-03 17:15:36 UTC
Comment on attachment 287433 [details] [review]
user-accounts: fix enterprise accounts deleting

commit 5790368643487778d7b7cfb40d082d0f912189f0

with the modified text from the Comment 17.
Comment 20 Ondrej Holy 2014-11-03 17:18:19 UTC
accountsservice version requirement was bumped by

commit 3f4669d48da1ec41fcddb618dc8ec53561e976d4