Did you update your webkitgtk package in the meanwhile?

The embedded web browser in gnome-online-accounts deletes all cookies to avoid these kinds of problems. I see that webkitgtk-2.16 has some changes around the API that we use to do that, namely webkit_cookie_manager_delete_all_cookies. It has been deprecated in favour of webkit_website_data_manager_clear. I am wondering if that has unintentionally broken the old API.

Yes: my webkitgtk4 packages (including -jsc and -plugin-process-gtk2) were updated a week ago, from version 2.14.5-1 to 2.16.0-1. I think I removed my accounts from GOA before then.

Thanks for confirming that! This is beginning to sound like a webkitgtk regression.

It does seem likely that this is a WebKit regression. webkit_cookie_manager_delete_all_cookies() now works by calling webkit_website_data_manager_clear(), but perhaps something is broken. It would be helpful to prepare a reduced test case and file a WebKit bug.

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

Why cache data at all though? This is easier:
https://git.gnome.org//browse/gnome-shell/tree/js/portalHelper/main.js#n144

In the meantime, is there at least a way for a user to manually delete WebKit cookies?

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

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

(In reply to krinkodot22 from comment #7)
> In the meantime, is there at least a way for a user to manually delete
> WebKit cookies?

$ rm ~/.cache/goa-1.0/cookies.sqlite*

(In reply to Michael Catanzaro from comment #4)
> It does seem likely that this is a WebKit regression.
> webkit_cookie_manager_delete_all_cookies() now works by calling
> webkit_website_data_manager_clear(), but perhaps something is broken. It
> would be helpful to prepare a reduced test case and file a WebKit bug.

I filed https://bugs.webkit.org/show_bug.cgi?id=175265

(In reply to Bastien Nocera from comment #6)
> Why cache data at all though? This is easier:
> https://git.gnome.org//browse/gnome-shell/tree/js/portalHelper/main.js#n144

Nice. Looks like a useful addition in webkitgtk-2.16.

However, that's not the reason we do these weird contortions with cookies. By default, WebKitCookieManager doesn't store the cookies persistently. So the default behaviour would've been just fine. Except for bug 694313.

Bug 694313 was about stealing cookies from the user's browser. Imagine that you are already logged into Facebook and you want to add your FB account in GOA. You could add the browser's cookies to GOA's web view and avoid having to log in again.

Back in the days of WebKit1, we'd get the SoupCookieJar from WebKit's SoupSession and add the cookies to it.

Sadly, WebKit2 doesn't have a way to get to the SoupSession. So we have this hack where a persistent SQLite-backed SoupCookieJarDB and WebKitCookieManager are pointed to the same path, and the jar is cleared when the WebKitWebView is constructed. No idea if this is prone to races or not, though, because the cookie stealing feature is currently unused.

The easiest solution might be to delete the SQLite file instead of trying to clear its contents. The other option is to drop the cookie stealing support. It has lain unused ever since it was added in 2013.

How do you know where to steal cookies from?

Currently Epiphany stores cookies under ~/.config/epiphany, but we'll move them to ~/.local/share/epiphany sooner or later, because we've discovered that nothing we store there is actually configuration. Do you steal cookies only from Epiphany?

WebKitGTK+ defaults to some other path, but probably no browser should be using that. Are you stealing cookies from the default path? What would be the point of that?

Keeping unused cookie stealing code around is probably not worth it. We used to have code to import cookies from the main Epiphany profile into Epiphany web apps, but deleted it after we found that broke certain Google domains. Unless you import *all* cookies without exception, it's fragile, because importing only a subset of cookies can break websites.

(In reply to Michael Catanzaro from comment #13)
> How do you know where to steal cookies from?

See bug 694313. :)

Emanuele Aina had a Chrome/Chromium extension to pull the cookies out of the web browser. It would feed them to Settings -> Online Accounts, which would feed them to GOA using goa_provider_set_preseed_data.

> Keeping unused cookie stealing code around is probably not worth it. We used
> to have code to import cookies from the main Epiphany profile into Epiphany
> web apps, but deleted it after we found that broke certain Google domains.
> Unless you import *all* cookies without exception, it's fragile, because
> importing only a subset of cookies can break websites.

Ok. That's good to know.

So, for the sake of being pedantic and not removing the goa_provider_set_preseed_data API this late in the cycle [*], I will workaround this by deleting the SQLite file instead of trying to empty its contents. I'll also remove the gnome-control-center part of the cookie-stealing code because I don't think anybody uses it.

Then for 3.27.x, I will drop the remaining code from GOA and bump libgoa-backend-1.0.so's soname. It's a semi-private interface only meant for g-c-c and g-i-s, so it's not a big deal.

CCing Emanuele, since we are talking about his code.

Created attachment 359634 [details] [review]
Proposed patch

This patch fixes the issue by using sqlite api directly

Created attachment 359635 [details] [review]
Improvment: delete cookies from persistent storage after getting credentials

Review of attachment 359634 [details] [review]:

This is a webkitgtk bug. See: https://bugs.webkit.org/show_bug.cgi?id=175265

A simpler option might be to just delete the entire cookies.sqlite file (comment 12). Doesn't that work? Going behind WebKit's back and directly messing with the the sqlite DB seems ... icky.

Review of attachment 359635 [details] [review]:

::: src/goabackend/goaoauth2provider.c
@@ +1109,3 @@
  out:
+  /* Give the webview a chance to cleanup its cookies ASAP */
+  gtk_container_remove (GTK_CONTAINER (vbox), grid);

How does this work? :) Is this related to the other patch where getting rid of the widget makes it easier to empty the DB using sqlite?

I definitely would not do anything to the database other than unlink(). You'd also need to unlink the WAL files.

I'm not sure it's worth committing a workaround at this point because we already have a patch in the WebKit bug and distros understand the importance of updating WebKit nowadays, we just need to land it.

(In reply to Debarshi Ray from comment #18)
> Review of attachment 359634 [details] [review] [review]:
> 
> This is a webkitgtk bug. See: https://bugs.webkit.org/show_bug.cgi?id=175265
> 
> A simpler option might be to just delete the entire cookies.sqlite file

In order to be able to delete the file we have to make sure the webview is not using it anymore, so we would have to delete it in construction and finalize

And we still have lots of race conditions if we run more than one webview at the same time, which we already had anyways.

Ideally we need new api in webkitgtk to inject cookies at runtime.

Or have one unique tmp file for each webview instance, I do not like this one because there is no way to ensure we will delete the file, if the process crash before finalizing the webview we would leave session cookies in the tmp file until the directory is cleaned up.

> (comment 12). Doesn't that work? Going behind WebKit's back and directly
> messing with the the sqlite DB seems ... icky.

I know, but isn't going trough soup api even more icky?
(For example when the webview adds a cookie, libsoup api does not see it)



(In reply to Debarshi Ray from comment #19)
> Review of attachment 359635 [details] [review] [review]:
> 
> ::: src/goabackend/goaoauth2provider.c
> @@ +1109,3 @@
>   out:
> +  /* Give the webview a chance to cleanup its cookies ASAP */
> +  gtk_container_remove (GTK_CONTAINER (vbox), grid);
> 
> How does this work? :) Is this related to the other patch where getting rid
> of the widget makes it easier to empty the DB using sqlite?

gnome-control-center does not destroy the webview until you re open the dialog, so clearing the cookies in the finalize method is pointless, also it does not finalize objects when the app is closed :(

(In reply to Michael Catanzaro from comment #20)
> I definitely would not do anything to the database other than unlink().
> You'd also need to unlink the WAL files.

I agree but that also include not doing anything trough soup API which in the end calls sqlite api just like my patch does, in the end all I did was cut the middle man, but I do agree its fells icky :)

> I'm not sure it's worth committing a workaround at this point because we
> already have a patch in the WebKit bug and distros understand the importance
> of updating WebKit nowadays, we just need to land it.

Right, besides there is a good chance distro will update webkit faster than goa.

In any case, until we have a way to add cookies to a WebKitCookieManager I think we need to clean up this code to use a different tmp file for each instance which should be deleted when the web view is finalized

(In reply to Juan Pablo Ugarte from comment #21)
> > A simpler option might be to just delete the entire cookies.sqlite file
> 
> In order to be able to delete the file we have to make sure the webview is
> not using it anymore, so we would have to delete it in construction and
> finalize
> 
> And we still have lots of race conditions if we run more than one webview at
> the same time, which we already had anyways.

I meant, deleting all the files in ~/.cache/goa-1.0/cookies* files in goa_web_view_constructed before calling webkit_cookie_manager_set_persistent_storage.
 
> Ideally we need new api in webkitgtk to inject cookies at runtime.

Umm... this bug is just about clearing out all existing cookies.

> Or have one unique tmp file for each webview instance, I do not like this
> one because there is no way to ensure we will delete the file, if the
> process crash before finalizing the webview we would leave session cookies
> in the tmp file until the directory is cleaned up.

Yes, that's too messy for a workaround. Plus as Michael said, it doesn't seem to be about one cookies.sqlite file. There are all those associated WAL files.

> > (comment 12). Doesn't that work? Going behind WebKit's back and directly
> > messing with the the sqlite DB seems ... icky.
> 
> I know, but isn't going trough soup api even more icky?
> (For example when the webview adds a cookie, libsoup api does not see it)

Well, webkitgtk uses libsoup, so it's not as bad as we directly messing with the database using sqlite. :)

> (In reply to Debarshi Ray from comment #19)
> > Review of attachment 359635 [details] [review] [review] [review]:
> > 
> > ::: src/goabackend/goaoauth2provider.c
> > @@ +1109,3 @@
> >   out:
> > +  /* Give the webview a chance to cleanup its cookies ASAP */
> > +  gtk_container_remove (GTK_CONTAINER (vbox), grid);
> > 
> > How does this work? :) Is this related to the other patch where getting rid
> > of the widget makes it easier to empty the DB using sqlite?
> 
> gnome-control-center does not destroy the webview until you re open the
> dialog, so clearing the cookies in the finalize method is pointless

Is this something new with the redesigned Online Accounts panel. (I have spotted some other problems with the way the code is structured.) Or was it so even before with the old UI?

(In reply to Debarshi Ray from comment #23)
> Well, webkitgtk uses libsoup, so it's not as bad as we directly messing with
> the database using sqlite. :)

But the SoupCookieJar used is an implementation detail of WebKit. E.g. in 2.18 it now uses the upstream SoupCookieJarDb, but in 2.16 it used a custom cookie jar that happened to use the same database schema. Regardless, you are relying on implementation details here. I'm also not sure of the implications of interacting with the database at the same time as WebKit. Or the implications of not using the same journaling mode (WAL) as WebKit.

I think this is a case where the best fix is to just do nothing... it's awkward as Juan clearly put a lot of effort into trying to fix this bug and has a patch that works, but there are costs to implementing this workaround, and WebKit will be fixed soon enough anyway. We do need to get that patch approved by Apple as it touches code that we're not supposed to change ourselves, but that will not be a problem.

(In reply to Debarshi Ray from comment #23)
> > gnome-control-center does not destroy the webview until you re open the
> > dialog, so clearing the cookies in the finalize method is pointless
> 
> Is this something new with the redesigned Online Accounts panel. (I have
> spotted some other problems with the way the code is structured.) Or was it
> so even before with the old UI?

No, this happens with the old UI, the dialog is not destroyed when its closed so if you open the control center, add a new goa acount and close it, the webview is never finalized, that is why I think its better to remove the webview in the same place that was added get_tokens_and_identity()

(In reply to Michael Catanzaro from comment #24)
> (In reply to Debarshi Ray from comment #23)
> > Well, webkitgtk uses libsoup, so it's not as bad as we directly messing with
> > the database using sqlite. :)
> 
> But the SoupCookieJar used is an implementation detail of WebKit. E.g. in
> 2.18 it now uses the upstream SoupCookieJarDb, but in 2.16 it used a custom
> cookie jar that happened to use the same database schema. Regardless, you
> are relying on implementation details here. I'm also not sure of the
> implications of interacting with the database at the same time as WebKit. Or
> the implications of not using the same journaling mode (WAL) as WebKit.

I had the feeling that Juan was referring to the hacks we do with SoupCookieJar for our "cookie stealing" code. See comment 12 and:
https://git.gnome.org/browse/gnome-online-accounts/tree/src/goabackend/goawebview.c#n519

That hack was suggested by Carlos as a "it might work, but who knows" thing. :)

I am getting more and more inclined to drop this entire feature. Nobody passes cookies from the browser to GOA in practice, it relies on undocumented behaviour, and this bug only exists because of it. Sadly, I couldn't axe it before 3.26.0.

Juan wants to re-use GOA's cookies for showing a web view for sharing. However, I am doubtful that it will work because GOA wants to always start with a clean cookie jar. If it is a matter of showing a web view for a full-fledged web login (as opposed to OAuth), I think we are better of just asking the user to enter her credentials, and keeping a separate cookie jar, etc.. That's what Documents does currently.

This is also going a bit off-topic now. :)

> I think this is a case where the best fix is to just do nothing... it's
> awkward as Juan clearly put a lot of effort into trying to fix this bug and
> has a patch that works, but there are costs to implementing this workaround,
> and WebKit will be fixed soon enough anyway.

Yeah. :/

(In reply to Debarshi Ray from comment #26)
> I am getting more and more inclined to drop this entire feature. Nobody
> passes cookies from the browser to GOA in practice, it relies on
> undocumented behaviour, and this bug only exists because of it. Sadly, I
> couldn't axe it before 3.26.0.

Yeah remove it, thats a good option if you do not mind breaking ABI. 

> Juan wants to re-use GOA's cookies for showing a web view for sharing.
> However, I am doubtful that it will work because GOA wants to always start
> with a clean cookie jar. If it is a matter of showing a web view for a

It does work, if you save the cookies form the login session you can reuse them in any browser, I tried it.

The problem is that I might need the cookie jar file hack to get the cookies from the webview unless it posible to get them from WebKitWebsiteDataManager

(In reply to Juan Pablo Ugarte from comment #27)
> > Juan wants to re-use GOA's cookies for showing a web view for sharing.
> > However, I am doubtful that it will work because GOA wants to always start
> > with a clean cookie jar. If it is a matter of showing a web view for a
> 
> It does work, if you save the cookies form the login session you can reuse
> them in any browser, I tried it.

I meant that GOA will keep nuking its cookie jar.

So (a) you'll have to save them in a separate cookie jar for your web view; (b) there is a very slim chance of a race between GOA deleting the cookies and you reading them; and (c) if the user has more than one account from a particular provider then you might hit a bug like this one.

> The problem is that I might need the cookie jar file hack to get the cookies
> from the webview unless it posible to get them from WebKitWebsiteDataManager

As far as I know, WK2 doesn't give you access to the actual SoupCookieJar. :/

Created attachment 359738 [details] [review]
Proposed path: use a different persistent storage file for each instance

I revived my first attempt and fixed the webview "leak" in get_tokens_and_identity() so that the tmp file gets deleted

(In reply to Debarshi Ray from comment #28)
> As far as I know, WK2 doesn't give you access to the actual SoupCookieJar. :/

It can't, since it lives in the network process.

If you need to read cookies then I'm afraid the best way is indeed what you're doing, using SQLite API directly. Modifying WebKitWebsiteDataManager or WebKitCookieManager to allow viewing the contents of cookies would be a significant undertaking.

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

Review of attachment 359738 [details] [review]:

As I mentioned before (comment 14 and comment 26), I'd rather drop the feature that requires this code, than try to work around the WebKitGTK+ bug. There's no need to take so much trouble, just to preserve a feature that nobody uses. :)

I filed a patch in bug 789469 to remove the gnome-control-center code. My plan is to gut the innards of goa_provider_set_preseed_data, deprecate it and keep it around as an empty shell to avoid breaking API/ABI. That should make it easier to backport to older stable branches.

Note that an API/ABI break isn't actually that expensive because this is part of the semi-private libgoa-backend-1.0.so that's only meant for privileged OS components like gnome-control-center and gnome-initial-setup. It's not something that apps are expected to consume.

Created attachment 362255 [details] [review]
Remove the option to preseed the providers & deprecate associated API

Here's what I'd do. Lightly tested so far.

Comment on attachment 362255 [details] [review]
Remove the option to preseed the providers & deprecate associated API

Pushed to master. I'll be cherry-picking to gnome-3-26 and gnome-3-24 shortly.

Let me know if something is off.

(In reply to Debarshi Ray from comment #34)
> I'll be cherry-picking to gnome-3-26 and gnome-3-24
> shortly.

Umm... probably it's best not to do that. On the crazy off chance that somebody somewhere is actually using it, it might be better to let distributors carry it downstream for the stable releases.

FYI pretty soon this will be possible to implement without any hacks since webkit will get new api to add/remove cookies at runtime

https://bugzilla.gnome.org/show_bug.cgi?id=781005