Created attachment 273848 [details]
user list

Created attachment 273849 [details]
del user

Created attachment 273850 [details]
bug

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...

There is another downstream bug report with info how to fix it:
https://bugzilla.redhat.com/show_bug.cgi?id=1060183#c6

Given that we only cache remote users (not really create them), it would seem logical to uncache them (instead of deleting them)

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...

This change for accountsservice is needed for:
https://bugs.freedesktop.org/show_bug.cgi?id=84091

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?

(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...

Created attachment 287432 [details] [review]
user-accounts: use common function for error messages

Created attachment 287433 [details] [review]
user-accounts: fix enterprise accounts deleting

(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?"

Review of attachment 287432 [details] [review]:

+

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.

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.

Thanks for the review, do you think "Are you sure you want to revoke remotely managed %s's account?" is better (and correct)?

Comment on attachment 287432 [details] [review]
user-accounts: use common function for error messages

commit ed13a62c1d17105bd40c110ddbfed2b15cfa1b48

Comment on attachment 287433 [details] [review]
user-accounts: fix enterprise accounts deleting

commit 5790368643487778d7b7cfb40d082d0f912189f0

with the modified text from the Comment 17.

accountsservice version requirement was bumped by

commit 3f4669d48da1ec41fcddb618dc8ec53561e976d4