Systems without HMI currently cannot use CardDAV/CalDAV services that requires OAuth2 authorization (existing provider is using gSSO which requires HMI). As alternative for such systems OAuth2 authorization can be made using refresh token obtained by user in different way (e.g. using gSSO on system with HMI).
Created attachment 105605 [details] [review] Proposed patch I'm not sure if check in SyncConfig::setPassword can be safetly removed, for now it's just commented out.
Comment on attachment 105605 [details] [review] Proposed patch Instead of copying GVariantCXX from signon.cpp, move it and related helper functions into a new src/syncevo/GVariant.[h|cpp] pair. Feel free to remove the commented code in src/syncevo/SyncConfig.cpp. Instead of repeating the boost::function<void (const std::string &newPassword)> type, better add a typedef for it in IdentityProvider.h. Last but not least, please add a src/backends/oauth2/README describing how to use the new provider. Of particular interest to users will be how they can extract the necessary refresh token from gSSO and GNOME Online Accounts.
Created attachment 105737 [details] [review] Patch with applied changes from review In previous patch I've forgotten to add change of SoupTransportAgent to use system default CA certificates if they were not provided by setSSL function.
I've split up the patch a bit and fixed a few things (extra white space at end of lines, missing API change in getOAuth2Bearer from signon.cpp, formatting and text of the README, compiler warnings about TransportAgent::CLOSED and INACTIVE, conditional compilation, compile flags). I renamed GVariant.[h/cpp] to GVariantSupport (to be consistent with other C++ utility files for C APIs) and dropped the hand-written auto_ptr in favor of the more flexible shared CXX wrapper. json.pc seems to be a legacy library (according to Debian Testing); I made the code compile with it and json-c.pc, the successor. This is all just nitpicking. The core idea and implementation is okay. Thanks for the patch. I'll attach my revised patch series for review. One thing I am uncertain about is the use of "refresh token" in the identity provider name. It's surprising that the "oauth2" backend installs a providerrefreshtoken.so. I think I would prefer to have refresh[_-]?token replaced by simply oauth2. Agreed? The handling of the command-line-only case without a permanent config was missing. It's a bit hard to test because it only triggers when we need a new refresh token, and I haven't seen that happen. I suspect what would happens is an exception from a non-writable config node, which is covered by the general catch all. I have also not tested how a failure to get an OAuth2 bearer token is reported. In particular I cannot guarantee that a sync fails with an error that indicates an authentication problem.
Created attachment 105906 [details] [review] refactoring
Created attachment 105907 [details] [review] identity provider + password
Created attachment 105908 [details] [review] souptransport + SSL
Created attachment 105909 [details] [review] soup transport init
Created attachment 105910 [details] [review] new oauth2 backend
Created attachment 105911 [details] [review] json compat
Created attachment 105912 [details] [review] enhanced README
Created attachment 105913 [details] [review] GVariant string hash
Created attachment 105914 [details] [review] simplify oauth2 backend config
Created attachment 105915 [details] [review] password update errors
> One thing I am uncertain about is the use of "refresh token" in the identity > provider name. It's surprising that the "oauth2" backend installs a > providerrefreshtoken.so. I think I would prefer to have refresh[_-]?token > replaced by simply oauth2. Agreed? Agreed > The handling of the command-line-only case without a permanent config was > missing. It's a bit hard to test because it only triggers when we need a new > refresh token, and I haven't seen that happen. I suspect what would happens > is an exception from a non-writable config node, which is covered by the > general catch all. I know that it's hard to trigger refresh token update (I also haven't seen that happen), to test refresh token update code during development I was just triggering update by myself each time that I was receiving access token. > I have also not tested how a failure to get an OAuth2 bearer token is > reported. In particular I cannot guarantee that a sync fails with an error > that indicates an authentication problem. Here is how such error looks like when exporting vcards [INFO] SoupTransport Failure: https://accounts.google.com/o/oauth2/token via libsoup: Bad Request [ERROR] OAuth2 request failed with error: invalid_grant [ERROR] addressbook: error code from SyncEvolution access denied (remote, status 403): logging into remote service failed: OAuth2 request failed with error: invalid_grant [ERROR] addressbook: reading items
Created attachment 105955 [details] [review] OAuth2 backend now installs provideroauth2.so
Created attachment 105956 [details] [review] new oauth2 backend OAuth2 backend now install provideroauth2.so
New patches looks fine for me.
(In reply to comment #17) > Created attachment 105956 [details] [review] [review] > new oauth2 backend > > OAuth2 backend now install provideroauth2.so That's only one part of what I wondered about. There's also USE_REFRESH_TOKEN, enble_refresh_token and last but not least, the "username=refresh-token:" prefix. Let me propose a different patch...
Created attachment 105981 [details] rename refresh-token -> oauth2
This was included in 1.4.99.4.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.