GNOME Bugzilla – Bug 755316
backends: Factorize common code in credential handling
Last modified: 2021-07-05 10:58:35 UTC
This series adds a few helpers to help remove duplicated code in the various backends.
Created attachment 311708 [details] [review] provider: Add goa_utils_get_credentials helper This helper gathers code which is duplicated among several password-based providers. Eventually, a GoaPasswordBasedProvider base class could be introduced similarly to what is done for the oauth based providers.
Created attachment 311709 [details] [review] providers: Use goa_utils_get_credentials()
Created attachment 311710 [details] [review] imap-smtp: Use goa_utils_get_credentials in the provider Less straightforward that the other ones as the username is not obtained through goa_account_get_identity() but comes from the config file. Also, the credentials GVariant was used twice, once for IMAP and once for SMTP. I chose to call goa_utils_get_credentials() twice instead.
Created attachment 311711 [details] [review] imap-smtp: Fix c&p typo in error message When failing to authenticate with the SMTP server using the SMTP login/password, the error message would mention the IMAP login, not the SMTP login.
Created attachment 311712 [details] [review] Add goa_object_skeleton_attach_password_based This code is copied and pasted between all providers implementing the GoaPasswordBased DBus interface. This commit adds a helper method so that the various backend can share the code instead of duplicating it.
Created attachment 311713 [details] [review] providers: Use goa_object_skeleton_attach_password_based helper This allows to remove quite a bit of code.
Created attachment 313522 [details] [review] provider: Add goa_utils_get_credentials helper This helper gathers code which is duplicated among several password-based providers. Eventually, a GoaPasswordBasedProvider base class could be introduced similarly to what is done for the oauth based providers.
Created attachment 313523 [details] [review] providers: Use goa_utils_get_credentials()
Created attachment 313524 [details] [review] imap-smtp: Use goa_utils_get_credentials in the provider Less straightforward that the other ones as the username is not obtained through goa_account_get_identity() but comes from the config file. Also, the credentials GVariant was used twice, once for IMAP and once for SMTP. I chose to call goa_utils_get_credentials() twice instead.
Created attachment 313525 [details] [review] imap-smtp: Fix c&p typo in error message When failing to authenticate with the SMTP server using the SMTP login/password, the error message would mention the IMAP login, not the SMTP login.
Created attachment 313526 [details] [review] Add goa_object_skeleton_attach_password_based This code is copied and pasted between all providers implementing the GoaPasswordBased DBus interface. This commit adds a helper method so that the various backend can share the code instead of duplicating it.
Created attachment 313527 [details] [review] providers: Use goa_object_skeleton_attach_password_based helper This allows to remove quite a bit of code.
Only change in the updated patches is that I added the bug reference to the commit logs
Created attachment 315737 [details] [review] provider: Add goa_utils_get_credentials helper This helper gathers code which is duplicated among several password-based providers. Eventually, a GoaPasswordBasedProvider base class could be introduced similarly to what is done for the oauth based providers.
Created attachment 315738 [details] [review] providers: Use goa_utils_get_credentials()
Created attachment 315739 [details] [review] imap-smtp: Use goa_utils_get_credentials in the provider Less straightforward that the other ones as the username is not obtained through goa_account_get_identity() but comes from the config file. Also, the credentials GVariant was used twice, once for IMAP and once for SMTP. I chose to call goa_utils_get_credentials() twice instead.
Created attachment 315740 [details] [review] imap-smtp: Fix c&p typo in error message When failing to authenticate with the SMTP server using the SMTP login/password, the error message would mention the IMAP login, not the SMTP login.
Created attachment 315741 [details] [review] Add goa_object_skeleton_attach_password_based This code is copied and pasted between all providers implementing the GoaPasswordBased DBus interface. This commit adds a helper method so that the various backend can share the code instead of duplicating it.
Created attachment 315742 [details] [review] providers: Use goa_object_skeleton_attach_password_based helper This allows to remove quite a bit of code.
Rebased on top of master to take into account "595cb4f Sprinkle some debug messages when handling D-Bus calls", no other changes.
Review of attachment 315737 [details] [review]: Thanks for the patches, Christophe. ::: src/goabackend/goautils.c @@ +628,3 @@ + + account = goa_object_peek_account (object); + username = goa_account_get_identity (account); Note that it is dangerous to use goa_object_peek_account and goa_account_get_identity here. We should use goa_account_get_account and goa_account_dup_identity. If you grep for HANDLE_METHOD_INVOCATIONS_IN_THREAD, then you will notice that this code gets used in a thread other than the one where "object" was created. If you happen to call goa_object_peek_account at the same instant when the org.gnome.OnlineAccounts.Account interface is removed from the GoaObject, then the code will crash. The reason it doesn't crash so much in practice is because we almost never remove the org.gnome.OnlineAccounts.Account interface, unlike ones like org.gnome.OnlineAccounts.Mail and org.gnome.OnlineAccounts.Photos which get toggled by the GtkSwitch in Settings -> Online Accounts. This is an error induced by cargo-culting the same code over and over and has been like this since forever. Recently we were discussing this in the context of bug 756498 Since you are touching this code, maybe take the opportunity to also use the right method calls? Or we can do it in a separate patch. Up to you. @@ +651,3 @@ + *out_username = username; + if (out_password) + *out_password = password; We already asserted that out_username and out_password can't be NULL, so the checks are not needed. Although it does seem to be a nice touch to be able to atleast pass a NULL out_username.
Review of attachment 315737 [details] [review]: ::: src/goabackend/goautils.c @@ +637,3 @@ + GOA_ERROR_FAILED, /* TODO: more specific */ + _("Did not find '%s' with identity ‘%s’ in credentials"), + id, username); g_set_error would be nicer. We won't have to worry about a NULL error, and we are already using it everywhere. (We should fix the few remaining uses of g_error_new.)
Review of attachment 315738 [details] [review]: ::: src/goabackend/goaexchangeprovider.c @@ +960,3 @@ + object, + "password", + &identity, We don't need 'identity' anymore. This is what made me think that being able to pass NULL would be nice. ::: src/goabackend/goaowncloudprovider.c @@ +1112,3 @@ + &password, + NULL, /* GCancellable */ + &error); Ditto.
Review of attachment 315739 [details] [review]: ::: src/goabackend/goaimapsmtpprovider.c @@ +326,3 @@ + object, + "imap-password", + NULL, You can't pass NULL the way things stand at the moment. :) @@ +390,3 @@ + object, + "smtp-password", + NULL, Ditto.
Review of attachment 315740 [details] [review]: Good catch. Thanks.
Review of attachment 315741 [details] [review]: I wonder what the pros and cons of adding a GoaPasswordProvider base class would be. These providers, by their very nature, don't have much in common other than the GetPassword implementations, but a base class would be consistent with the way we handle the OAuth and OAuth2. ::: src/goabackend/goaobjectskeletonutils.c @@ +60,3 @@ + /* FIXME: is this going to be fine for owncloud/... ? */ + /* used to be a hardcoded "password" */ + id, Both the Exchange and ownCloud providers do this. I checked all known users, and evolution-data-server passes "" instead of "password" as the ID. So, maybe, add a check like this for the time being: if (id == NULL || id[0] == '\0') id = "password"; That would make life easier for developers who might have a mismatch between their g-o-a and e-d-s versions. In the meantime we can make e-d-s pass "password".
Comment on attachment 315740 [details] [review] imap-smtp: Fix c&p typo in error message Pushed to master, gnome-3-18, gnome-3-16 and gnome-3-14.
Created attachment 316926 [details] [review] provider: Add goa_utils_get_credentials helper This helper gathers code which is duplicated among several password-based providers. Eventually, a GoaPasswordBasedProvider base class could be introduced similarly to what is done for the oauth based providers. The addition of this new helper is used as an opportunity for small cleanups in the code which was copied/pasted between the various providers: - g_set_error is used instead of g_error_new - goa_object_get_account and goa_account_dup_identity are used instead of goa_object_peek_account and goa_account_get_identity as using the former has some thread-safety issues (see bug #755316 for details)
Created attachment 316927 [details] [review] providers: Use goa_utils_get_credentials()
Created attachment 316928 [details] [review] imap-smtp: Use goa_utils_get_credentials in the provider Less straightforward that the other ones as the username is not obtained through goa_account_get_identity() but comes from the config file. Also, the credentials GVariant was used twice, once for IMAP and once for SMTP. I chose to call goa_utils_get_credentials() twice instead.
Created attachment 316929 [details] [review] Add goa_object_skeleton_attach_password_based This code is copied and pasted between all providers implementing the GoaPasswordBased DBus interface. This commit adds a helper method so that the various backend can share the code instead of duplicating it.
Created attachment 316930 [details] [review] providers: Use goa_object_skeleton_attach_password_based helper This allows to remove quite a bit of code.
Review of attachment 315741 [details] [review]: I've resent the patches without trying your base class suggestion, I'll try looking into it, but not sure when I will have time to spend on that. ::: src/goabackend/goaobjectskeletonutils.c @@ +60,3 @@ + /* FIXME: is this going to be fine for owncloud/... ? */ + /* used to be a hardcoded "password" */ + id, Empathy is also passing "" so I've added the check and a g_warning.
Review of attachment 316927 [details] [review]: ::: src/goabackend/goautils.c @@ -619,3 @@ - g_return_val_if_fail (out_username != NULL, FALSE); - g_return_val_if_fail (out_password != NULL, FALSE); - Sorry, this hunk is out of place and should go in the previous commit
Review of attachment 316926 [details] [review]: Thanks Christophe. A few small things: ::: src/goabackend/goautils.c @@ +633,3 @@ + { + g_set_error (error, GOA_ERROR, GOA_ERROR_FAILED, /* TODO: more specific */ + _("Did not find '%s' with identity ‘%s’ in credentials"), Nitpick. Don't use the ASCII single quotes around the first %s. Two reasons: (i) The strings that we are about to replace don't use the first set of quotes. (ii) More importantly, if we really want to use quotes, then we should use Unicode ones. Philip spent some time fixing those up (see bug 707435), so it is better not to regress on that front. @@ +636,3 @@ + id, username); + g_free (username); + username = NULL; I will sleep better if we freed them in only one spot. We can accomplish that by assigning the values to the 'out' variables just before the 'out:' target, and setting the local variables to NULL. @@ +644,3 @@ +out: + if (account != NULL) + g_object_unref (account); g_clear_object will make this simpler. @@ +646,3 @@ + g_object_unref (account); + if (credentials != NULL) + g_variant_unref (credentials); Ditto, but g_clear_pointer.
Review of attachment 316927 [details] [review]: ::: src/goabackend/goaowncloudprovider.c @@ +347,3 @@ gboolean accept_ssl_errors; gboolean ret; + gchar *username; This should be NULL initialized. Otherwise we are relying on goa_utils_get_credentials to set it to NULL even when it failed. That is a bit fragile. @@ +1113,3 @@ + NULL, /* GCancellable */ + &error); + if (!ret) Nitpick. We could directly check the return value inside the if (...).
Created attachment 319492 [details] [review] utils: Add goa_utils_get_credentials helper I took the liberty to address the above comments and also moved the chunk from the other patch.
Created attachment 319493 [details] [review] exchange, lastfm, owncloud: Use goa_utils_get_credentials()
Review of attachment 316928 [details] [review]: ::: src/goabackend/goaimapsmtpprovider.c @@ +387,3 @@ } + ret = goa_utils_get_credentials (provider, Oops! The older code had a bug lurking in here. If the g_variant_lookup failed, we would throw an error but still return TRUE. :( Let's fix this in a separate patch so that we can backport it to older branches.
Created attachment 319499 [details] [review] imap-smtp: Use goa_utils_get_credentials in the provider I took the liberty to make the above adjustment.
(In reply to Debarshi Ray from comment #39) > Review of attachment 316928 [details] [review] [review]: > > ::: src/goabackend/goaimapsmtpprovider.c > @@ +387,3 @@ > } > > + ret = goa_utils_get_credentials (provider, > > Oops! The older code had a bug lurking in here. If the g_variant_lookup > failed, we would throw an error but still return TRUE. :( > > Let's fix this in a separate patch so that we can backport it to older > branches. I filed bug 760991 for it.
Created attachment 319740 [details] [review] Add goa_object_skeleton_attach_password_based This code is copied and pasted between all providers implementing the GoaPasswordBased DBus interface. This commit adds a helper method so that the various backend can share the code instead of duplicating it.
Created attachment 319741 [details] [review] providers: Use goa_object_skeleton_attach_password_based helper This allows to remove quite a bit of code.
I've rebased the 2 remaining patches on top of master. Let me know if you'd prefer if I explore now the base class suggestion you made in comment #26.
Alternatively, now that the goa_utils_get_credentials() patches went in, these last 2 patches could also be managed as part of a larger series I still have pending which add goa_object_skeleton_attach_{calendar,files,contacts,...} helpers.
Created attachment 321309 [details] [review] backend: Add and use GoaPasswordBased base class This code is copied and pasted between all providers implementing the GoaPasswordBased DBus interface. This commit adds a base GoaPasswordBased class so that the various backend can share the code instead of duplicating it.
Created attachment 362020 [details] [review] backend: Add and use GoaPasswordBased base class This code is copied and pasted between all providers implementing the GoaPasswordBased DBus interface. This commit adds a base GoaPasswordBased class so that the various backend can share the code instead of duplicating it.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-online-accounts/-/issues/ Thank you for your understanding and your help.