*** Bug 686426 has been marked as a duplicate of this bug. ***

I think this would be a really useful addition to gnome-online-accounts. We do have easy network monitoring support in glib now, so it shouldn't be hard to do.

*** Bug 746414 has been marked as a duplicate of this bug. ***

*** Bug 732940 has been marked as a duplicate of this bug. ***

Created attachment 304158 [details] [review]
daemon: Don't leak the GoaProvider

Created attachment 304159 [details] [review]
daemon: Use g_clear_object wherever applicable

Created attachment 304160 [details] [review]
daemon: Use g_list_free_full wherever applicable

Created attachment 304161 [details] [review]
daemon: Remove redundant NULL check

Created attachment 304162 [details] [review]
daemon: Remove redundant function call

Created attachment 304163 [details] [review]
daemon: Check & refresh credentials during startup and network changes

Created attachment 304170 [details] [review]
kerberos: Mark EnsureCredentials failures as authorization errors

Review of attachment 304158 [details] [review]:

looks right, sort of surprised this didn't get picked up in previous rounds of leak fixes.

::: src/daemon/goadaemon.c
@@ +1202,2 @@
  out:
+  g_clear_object (&provider);

one idea is to drop this and use g_autoptr like the cool kids do these days.

Review of attachment 304159 [details] [review]:

::: src/daemon/goadaemon.c
@@ +509,1 @@
   g_object_unref (account);

could do account, too, just to be consistent (or switch to g_autoptr)

Review of attachment 304160 [details] [review]:

looks right to me. there's 3 other places outside of goadaemon.c that could be updated, too, though.

Review of attachment 304161 [details] [review]:

++

Review of attachment 304162 [details] [review]:

sure. you could go a step further and call goa_object_peek_account at the top of the function, to deduplicate even more (though that has the disadvantage of it getting called in some failure cases when it's not needed)

Review of attachment 304163 [details] [review]:

looks fine to me from a cursory read through

::: src/daemon/goadaemon.c
@@ +173,3 @@
 
+static gboolean
+queue_check_credentials_timeout (gpointer user_data)

the other timeout in this file is of the form on_..._timeout so this should probably be called on_check_credentials_timeout

@@ +192,3 @@
+    }
+
+  self->credentials_timeout_id = g_timeout_add (200, queue_check_credentials_timeout, self);

I like the idea of a using a timeout to rate limit the checks in the face of a network fumbling up, but i wonder if the interval is right. 200ms seems arbitrary is might be okay, but since we're being arbitrary we might as well do g_timeout_add_seconds(1, ...) so the wake up is aligned with other wake ups...

@@ +1195,3 @@
+          g_dbus_method_invocation_take_error (data->invocation, error);
+          error = NULL;
+        }

you could move the g_clear_error here

@@ +1215,3 @@
     }
+
+  g_clear_error (&error);

then you wouldn't need it here (though I don't have really strong opinions on that)

@@ +1276,3 @@
+      account = goa_object_peek_account (object);
+      if (account == NULL)
+        goto continue_objects;

this could just be "continue;"

@@ +1281,3 @@
+      provider = goa_provider_get_for_provider_type (provider_type);
+      if (provider == NULL)
+        goto continue_objects;

this could just be "continue;"

@@ +1289,3 @@
+                                       ensure_data_new (self, object, NULL));
+
+    continue_objects:

this isn't needed, all the cases goto are used have a NULL provider

Review of attachment 304170 [details] [review]:

so looks like this is fallout from commit 7ba73645e6068935f331969e14d56a39544ebca5.

prior to that commit it did:

      translate_error (&lookup_error);
      g_set_error_literal (error,
                           GOA_ERROR,
                           GOA_ERROR_NOT_AUTHORIZED,
                           lookup_error->message);
      g_error_free (lookup_error);

::: src/goabackend/goakerberosprovider.c
@@ +1407,3 @@
+            {
+              (*error)->domain = GOA_ERROR;
+              (*error)->code = GOA_ERROR_NOT_AUTHORIZED;

which i like slightly better than this ^ but whatever.

(In reply to Ray Strode [halfline] from comment #12)
> Review of attachment 304158 [details] [review] [review]:
> 
> looks right, sort of surprised this didn't get picked up in previous rounds
> of leak fixes.

We probably didn't check the EnsureCredentials code path.

> ::: src/daemon/goadaemon.c
> @@ +1202,2 @@
>   out:
> +  g_clear_object (&provider);
> 
> one idea is to drop this and use g_autoptr like the cool kids do these days.

Yes, I would love to switch to g_autoptr and G_DECLARE_*, but I want to hold back a bit so that it is still easy to back port patches to older branches. My motivation threshold for older non-RHEL branches is "as long as cherry-pick works". :)

(In reply to Ray Strode [halfline] from comment #13)
> Review of attachment 304159 [details] [review] [review]:
> 
> ::: src/daemon/goadaemon.c
> @@ +509,1 @@
>    g_object_unref (account);
> 
> could do account, too, just to be consistent (or switch to g_autoptr)

Ok. Consistency is good.

Created attachment 304239 [details] [review]
daemon: Use g_clear_object wherever applicable

Committed after making the suggested change to retain consistency.

(In reply to Ray Strode [halfline] from comment #17)
> Review of attachment 304163 [details] [review] [review]:
> 
> looks fine to me from a cursory read through
> 
> ::: src/daemon/goadaemon.c
> @@ +173,3 @@
>  
> +static gboolean
> +queue_check_credentials_timeout (gpointer user_data)
> 
> the other timeout in this file is of the form on_..._timeout so this should
> probably be called on_check_credentials_timeout

Good point. Fixed.

> @@ +192,3 @@
> +    }
> +
> +  self->credentials_timeout_id = g_timeout_add (200,
> queue_check_credentials_timeout, self);
> 
> I like the idea of a using a timeout to rate limit the checks in the face of
> a network fumbling up, but i wonder if the interval is right. 200ms seems
> arbitrary is might be okay, but since we're being arbitrary we might as well
> do g_timeout_add_seconds(1, ...) so the wake up is aligned with other wake
> ups...

Good point. I used 200 ms to mimic the other timeout that we use for file monitor changes. Would it be a big deal if we change that to 1 second too? Probably not, I guess.

> @@ +1195,3 @@
> +          g_dbus_method_invocation_take_error (data->invocation, error);
> +          error = NULL;
> +        }
> 
> you could move the g_clear_error here
> 
> @@ +1215,3 @@
>      }
> +
> +  g_clear_error (&error);
> 
> then you wouldn't need it here (though I don't have really strong opinions
> on that)

Done.

> @@ +1276,3 @@
> +      account = goa_object_peek_account (object);
> +      if (account == NULL)
> +        goto continue_objects;
> 
> this could just be "continue;"
> 
> @@ +1281,3 @@
> +      provider = goa_provider_get_for_provider_type (provider_type);
> +      if (provider == NULL)
> +        goto continue_objects;
> 
> this could just be "continue;"
> 
> @@ +1289,3 @@
> +                                       ensure_data_new (self, object,
> NULL));
> +
> +    continue_objects:
> 
> this isn't needed, all the cases goto are used have a NULL provider

Fixed.

Created attachment 304257 [details] [review]
kerberos: Mark EnsureCredentials failures as authorization errors

Changed the structure to match the original style.

Created attachment 304262 [details] [review]
client, identity: Use g_list_free_full wherever applicable

Created attachment 304263 [details] [review]
identity: Simplify the destruction

Created attachment 304264 [details] [review]
identity: Chain up during dispose

Review of attachment 304257 [details] [review]:

++

Review of attachment 304262 [details] [review]:

++

Created attachment 304265 [details] [review]
daemon: Check & refresh credentials during startup and network changes

Committed after making the suggested changes - changed the timeout to 1s, got rid of an unnecessary goto, and moved the g_clear_error higher up in the function.

Review of attachment 304263 [details] [review]:

This is okay (though keeping it all in dispose is okay too, not sure what's better)

Review of attachment 304264 [details] [review]:

oh weird, good catch

Thanks for all the review, halfline!

Thanks for implementing this, guys.

Created attachment 304279 [details] [review]
[gnome-3-16] kerberos: Fix ensure credentials when identity isn't yet known

Committed after reviewing and testing in #gnome-os on GIMPNet. This particular issue got resolved in master as a side-effect of commit 7ba73645e6068935f331969e14d56a39544ebca5

(In reply to Kamil Páral from comment #33)
> Thanks for implementing this, guys.

You're welcome. Apologies for taking so long.

Thanks for the fix, can't wait to use it. An idea of the time we've to wait to get it released? Thanks again.

(In reply to romu from comment #36)
> Thanks for the fix, can't wait to use it. An idea of the time we've to wait
> to get it released? Thanks again.

Next 3.6.x bug-fix release. Maybe 3.14.x too.

(In reply to Debarshi Ray from comment #37)
> (In reply to romu from comment #36)
> > Thanks for the fix, can't wait to use it. An idea of the time we've to wait
> > to get it released? Thanks again.
> 
> Next 3.6.x bug-fix release. Maybe 3.14.x too.

I'm guessing you mean the next 3.16 release.

(In reply to Bastien Nocera from comment #38)
> (In reply to Debarshi Ray from comment #37)
> > (In reply to romu from comment #36)
> > > Thanks for the fix, can't wait to use it. An idea of the time we've to wait
> > > to get it released? Thanks again.
> > 
> > Next 3.6.x bug-fix release. Maybe 3.14.x too.
> 
> I'm guessing you mean the next 3.16 release.

Yes, of course. Next 3.16.x release. Sorry for the confusion.

Hi,
Obviously the fix has been release because I don't need to manually refresh my Windows credentials anymore. So, I just want to say THANK YOU! This changes a bit the life when working with Gnome within a Windows network.

Hello,
For some days, I experience regressions in this area on F25. This morning I had to switch off/on again the "Use for Network Resources" switch of my Kerberos account to be able to connect some of my network shared folders. Could we re-open this bug?

(In reply to romu from comment #41)
> Hello,
> For some days, I experience regressions in this area on F25. This morning I
> had to switch off/on again the "Use for Network Resources" switch of my
> Kerberos account to be able to connect some of my network shared folders.
> Could we re-open this bug?

Why do you think it's the same problem that you're experiencing?
You're better off filing a new bug.

And a year and a half after...

@Bastien, is there any real need to be so harsh in your answer? I do what I can do to participate to Gnome: reporting problems, and try to help on diagnosis, etc. But I can also leave and take more benefit of my time than reading so unpleasant messages.

And I think this could be the same problem because I'm on the user side, not the developer one. And from the user point of view, on a pure functional aspect, this is the same problem...a regression.

In the code this may be a different bug, but I'm not in the code.

(In reply to romu from comment #41)
> For some days, I experience regressions in this area on F25. This morning I
> had to switch off/on again the "Use for Network Resources" switch of my
> Kerberos account to be able to connect some of my network shared folders.
> Could we re-open this bug?

Yes, as Bastien said, it would be better to open a new bug. I doubt it is the same problem as this one.

Were there some changes to the networking that made you toggle the switch? eg., you turned on a VPN or connected to WiFi or Ethernet. Whenever we refresh the credentials due to networking changes, we log a debug message. So, if you had G_MESSAGES_DEBUG=all set in your environment, you will see this in the logs:
  "Calling EnsureCredentials due to network changes"

Those are some of the things I would check when filing the bug.

(In reply to romu from comment #44)
> @Bastien, is there any real need to be so harsh in your answer?

You're reading it wrongly. It's not harsh, it's a question.

(In reply to romu from comment #45)
> And I think this could be the same problem because I'm on the user side, not
> the developer one. And from the user point of view, on a pure functional
> aspect, this is the same problem...a regression.
> 
> In the code this may be a different bug, but I'm not in the code.

If you're only seeing the problem a year and a half after your own last message, where you tested things, it's not the same problem. GNOME releases every 6 months, which would mean that you found a problem 3 releases later. You might want to link to this bug, but reopening it will not help (it makes the original bug and the follow-up problem harder to read by carrying around nearly 50 comments).

Looking forward to your new bug, thanks.

ok, let's keep calm.

I'll take some few days to diagnose a bit more and give you more insight of what happens to provide in the bug report...if needed. I didn't change anything on my network, but may have changed the wifi network I connect (we run 2 wifi networks).

Newbie question: how can I set G_MESSAGES_DEBUG=all?

(In reply to romu from comment #48)
> Newbie question: how can I set G_MESSAGES_DEBUG=all?

Easiest option is to run goa-daemon as:
$ G_MESSAGES_DEBUG=all /path/to/goa-daemon --replace &