Bug 83396 - Support for OAuth2 authorization in systems without HMI
Summary: Support for OAuth2 authorization in systems without HMI
Status: RESOLVED FIXED
Alias: None
Product: SyncEvolution
Classification: Unclassified
Component: CalDAV/CardDAV (show other bugs)
Version: 1.3.99.3
Hardware: Other All
: medium enhancement
Assignee: Mateusz Polrola
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-02 11:46 UTC by Mateusz Polrola
Modified: 2014-09-24 10:52 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Proposed patch (24.36 KB, patch)
2014-09-02 11:47 UTC, Mateusz Polrola
Details | Splinter Review
Patch with applied changes from review (34.06 KB, patch)
2014-09-04 12:17 UTC, Mateusz Polrola
Details | Splinter Review
refactoring (13.17 KB, patch)
2014-09-08 18:46 UTC, Patrick Ohly
Details | Splinter Review
identity provider + password (7.64 KB, patch)
2014-09-08 18:46 UTC, Patrick Ohly
Details | Splinter Review
souptransport + SSL (1.56 KB, patch)
2014-09-08 18:47 UTC, Patrick Ohly
Details | Splinter Review
soup transport init (862 bytes, patch)
2014-09-08 18:47 UTC, Patrick Ohly
Details | Splinter Review
new oauth2 backend (19.36 KB, patch)
2014-09-08 18:48 UTC, Patrick Ohly
Details | Splinter Review
json compat (1.47 KB, patch)
2014-09-08 18:48 UTC, Patrick Ohly
Details | Splinter Review
enhanced README (2.16 KB, patch)
2014-09-08 18:48 UTC, Patrick Ohly
Details | Splinter Review
GVariant string hash (2.07 KB, patch)
2014-09-08 18:49 UTC, Patrick Ohly
Details | Splinter Review
simplify oauth2 backend config (5.23 KB, patch)
2014-09-08 18:49 UTC, Patrick Ohly
Details | Splinter Review
password update errors (1.96 KB, patch)
2014-09-08 18:50 UTC, Patrick Ohly
Details | Splinter Review
OAuth2 backend now installs provideroauth2.so (19.31 KB, patch)
2014-09-09 07:32 UTC, Mateusz Polrola
Details | Splinter Review
new oauth2 backend (19.31 KB, patch)
2014-09-09 07:33 UTC, Mateusz Polrola
Details | Splinter Review
rename refresh-token -> oauth2 (9.15 KB, text/plain)
2014-09-09 13:13 UTC, Patrick Ohly
Details

Description Mateusz Polrola 2014-09-02 11:46:19 UTC
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).
Comment 1 Mateusz Polrola 2014-09-02 11:47:29 UTC
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 2 Patrick Ohly 2014-09-03 13:32:34 UTC
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.
Comment 3 Mateusz Polrola 2014-09-04 12:17:41 UTC
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.
Comment 4 Patrick Ohly 2014-09-08 18:45:40 UTC
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.
Comment 5 Patrick Ohly 2014-09-08 18:46:21 UTC
Created attachment 105906 [details] [review]
refactoring
Comment 6 Patrick Ohly 2014-09-08 18:46:50 UTC
Created attachment 105907 [details] [review]
identity provider + password
Comment 7 Patrick Ohly 2014-09-08 18:47:09 UTC
Created attachment 105908 [details] [review]
souptransport + SSL
Comment 8 Patrick Ohly 2014-09-08 18:47:36 UTC
Created attachment 105909 [details] [review]
soup transport init
Comment 9 Patrick Ohly 2014-09-08 18:48:03 UTC
Created attachment 105910 [details] [review]
new oauth2 backend
Comment 10 Patrick Ohly 2014-09-08 18:48:26 UTC
Created attachment 105911 [details] [review]
json compat
Comment 11 Patrick Ohly 2014-09-08 18:48:49 UTC
Created attachment 105912 [details] [review]
enhanced README
Comment 12 Patrick Ohly 2014-09-08 18:49:20 UTC
Created attachment 105913 [details] [review]
GVariant string hash
Comment 13 Patrick Ohly 2014-09-08 18:49:43 UTC
Created attachment 105914 [details] [review]
simplify oauth2 backend config
Comment 14 Patrick Ohly 2014-09-08 18:50:02 UTC
Created attachment 105915 [details] [review]
password update errors
Comment 15 Mateusz Polrola 2014-09-09 06:40:13 UTC
> 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
Comment 16 Mateusz Polrola 2014-09-09 07:32:16 UTC
Created attachment 105955 [details] [review]
OAuth2 backend now installs provideroauth2.so
Comment 17 Mateusz Polrola 2014-09-09 07:33:46 UTC
Created attachment 105956 [details] [review]
new oauth2 backend

OAuth2 backend now install provideroauth2.so
Comment 18 Mateusz Polrola 2014-09-09 08:36:02 UTC
New patches looks fine for me.
Comment 19 Patrick Ohly 2014-09-09 13:11:52 UTC
(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...
Comment 20 Patrick Ohly 2014-09-09 13:13:07 UTC
Created attachment 105981 [details]
rename refresh-token -> oauth2
Comment 21 Patrick Ohly 2014-09-24 10:52:30 UTC
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.