unity-scopes-api

Merge lp:~mardy/unity-scopes-api/clientid-1554040 into lp:unity-scopes-api

clientid-1554040
Merge into trunk

Proposed by Alberto Mardegan on 2016-03-10

Status:

Superseded

Proposed branch:

lp:~mardy/unity-scopes-api/clientid-1554040

Merge into:

lp:unity-scopes-api

Diff against target:

327 lines (+133/-12)

8 files modified

debian/VERSION (+1/-1)
debian/control.in (+1/-0)
doc/tutorial.dox (+5/-0)
include/unity/scopes/OnlineAccountClient.h (+16/-0)
include/unity/scopes/internal/OnlineAccountClientImpl.h (+3/-0)
src/scopes/OnlineAccountClient.cpp (+10/-1)
src/scopes/internal/OnlineAccountClientImpl.cpp (+26/-2)
test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp (+71/-8)

To merge this branch:

bzr merge lp:~mardy/unity-scopes-api/clientid-1554040

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2016-03-16
Marcus Tomlinson (community)		2016-03-10	Approve on 2016-03-16
Review via email: mp+288625@code.launchpad.net

This proposal has been superseded by a proposal from 2016-03-16.

Commit message

Allow clients to specify authentication parameters

Description of the change

Allow clients to specify authentication parameters

Please advise on how to set the versioning so that other packages can depend on these changes.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-03-11:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/646/rebuild

review: Approve (continuous-integration)

Revision history for this message

Marcus Tomlinson (marcustomlinson) wrote on 2016-03-14:

Sorry that this MP has not yet been reviewed, our team is swamped at the moment with OTA10 work. Please bare with us.

Revision history for this message

Marcus Tomlinson (marcustomlinson) wrote on 2016-03-14:

Thanks for the contribution Mardy! Please could you address the following:

1) Please update:

1.0.3

to:

1.0.4

in:

/debian/VERSION

2) 3 inline comments

review: Needs Fixing

lp:~mardy/unity-scopes-api/clientid-1554040 updated on 2016-03-15

361. By Alberto Mardegan on 2016-03-15: Tests, version and docs
362. By Alberto Mardegan on 2016-03-15: Clear access token when service is disabled

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-03-15:

FAILED: Continuous integration, rev:361
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/647/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-amd64-ci/155/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-armhf-ci/146/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-i386-ci/147/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-xenial-amd64-ci/7/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-xenial-armhf-ci/7/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-xenial-i386-ci/7/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/647/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-03-15:

FAILED: Continuous integration, rev:362
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/648/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-amd64-ci/156/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-armhf-ci/147/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-vivid-i386-ci/148/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-xenial-amd64-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-xenial-armhf-ci/8/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-xenial-i386-ci/8/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/648/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Marcus Tomlinson (marcustomlinson) wrote on 2016-03-16:

Thanks for the changes to version and the docs, looks good. Unfortunately, it looks like OnlineAccountClient_test is broken now:

58: Test command: /tmp/buildd/unity-scopes-api-1.0.3+16.04.20160209bzr362pkg0xenial156/obj-x86_64-linux-gnu/test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test
58: Test timeout computed to be: 1500
58: DBus daemon: unix:abstract=/tmp/dbus-muDbEkqdhF,guid=1a897736d3729d533f35d82d56e83f82
58: DBusMock: Started with PID: 13382
58: /usr/bin/python3: No module named dbusmock
58: DBusMock: Exited with status 256
58:
58: (process:13375): GLib-CRITICAL **: Source ID 6 was not found when attempting to remove it
58:
58: (process:13375): libdbustest-CRITICAL **: Unable to get DBusMock started within 3 seconds

review: Needs Fixing

Revision history for this message

Marcus Tomlinson (marcustomlinson) wrote on 2016-03-16:

As we discussed yesterday, if we could just get coverage of your new logic simply by constructing the main OnlineAccountClient object with a few dummy auth_params (an int, a string, a bool, a double, and one unsupported type), this branch should be good to go.

The changes are small enough that my only real concern here is the memory management.

lp:~mardy/unity-scopes-api/clientid-1554040 updated on 2016-03-16

363. By Alberto Mardegan on 2016-03-16: Add missing dependency

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-03-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/649/rebuild

review: Approve (continuous-integration)

lp:~mardy/unity-scopes-api/clientid-1554040 updated on 2016-03-16

364. By Alberto Mardegan on 2016-03-16: Add tests for double and unsupported types
365. By Alberto Mardegan on 2016-03-16: Don't put null elements in auth parameters

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-03-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/650/rebuild

review: Approve (continuous-integration)

Revision history for this message

Marcus Tomlinson (marcustomlinson) wrote on 2016-03-16:

One last small fix (inline)

review: Needs Fixing

lp:~mardy/unity-scopes-api/clientid-1554040 updated on 2016-03-16

366. By Alberto Mardegan on 2016-03-16: remove endl

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-03-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/651/rebuild

review: Approve (continuous-integration)

Revision history for this message

Marcus Tomlinson (marcustomlinson) wrote on 2016-03-16:

review: Approve

lp:~mardy/unity-scopes-api/clientid-1554040 updated on 2016-03-16

367. By Alberto Mardegan on 2016-03-16

merge devel

[ Pawel Stolowski ]
* New RangeInputFilter.
[ Michi Henning ]
* Changed ABI compliance testing to use abigail.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-03-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/652/rebuild

review: Approve (continuous-integration)

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alberto Mardegan

Unity Team

 === modified file 'debian/VERSION'
 --- debian/VERSION	2016-02-09 10:26:20 +0000
 +++ debian/VERSION	2016-03-16 12:08:30 +0000
@@ -1,1 +1,1 @@
--1.0.3
++1.0.4
 === modified file 'debian/control.in'
 --- debian/control.in	2015-11-13 04:05:00 +0000
 +++ debian/control.in	2016-03-16 12:08:30 +0000
@@ -32,6 +32,7 @@
                 lttng-tools,
                 pkg-config,
                 python3:any,
++               python3-dbusmock,
                 python-tornado,
                 valgrind,
  Standards-Version: 3.9.6
 === modified file 'doc/tutorial.dox'
 --- doc/tutorial.dox	2015-10-29 02:35:50 +0000
 +++ doc/tutorial.dox	2016-03-16 12:08:30 +0000
@@ -1120,6 +1120,11 @@
  specify our account service name, service type, and provider name (These correspond to the values of the "service_id",
  "type", and "provider" entries in your .service file).
++The constructor accepts a fourth optional parameter which can be used to specify a dictionary of authentication data,
++whose contents will complement (or override) those authentication parameters specified in the `<template>` element of
++the \link oa_service `.service` file\endlink. It can be used in those rare cases where the authentication parameters are
++known only at runtime.
++
  Via this object we can get the statuses of all account services, set a callback for status updates, and register
  results and widgets that require authorization (See: unity::scopes::OnlineAccountClient class documentation for more
  detail).
 === modified file 'include/unity/scopes/OnlineAccountClient.h'
 --- include/unity/scopes/OnlineAccountClient.h	2015-01-19 12:15:58 +0000
 +++ include/unity/scopes/OnlineAccountClient.h	2016-03-16 12:08:30 +0000
@@ -94,6 +94,22 @@
                          std::string const& provider_name,
                          MainLoopSelect main_loop_select = CreateInternalMainLoop);
++    /**
++    \brief Create OnlineAccountClient for the specified account service.
++    \param service_name The name of the service (E.g. "com.ubuntu.scopes.youtube_youtube").
++    \param service_type The type of service (E.g. "sharing").
++    \param provider_name The name of the service provider (E.g. "google").
++    \param auth_params Authentication parameters; this can be used to pass the
++    OAuth client keys, for example.
++    \param main_loop_select Indicates whether or not an external main loop exists
++                            (see OnlineAccountClient::MainLoopSelect).
++    */
++    OnlineAccountClient(std::string const& service_name,
++                        std::string const& service_type,
++                        std::string const& provider_name,
++                        VariantMap const& auth_params,
++                        MainLoopSelect main_loop_select = CreateInternalMainLoop);
++
      /// @cond
      ~OnlineAccountClient();
      /// @endcond
 === modified file 'include/unity/scopes/internal/OnlineAccountClientImpl.h'
 --- include/unity/scopes/internal/OnlineAccountClientImpl.h	2014-11-03 05:31:30 +0000
 +++ include/unity/scopes/internal/OnlineAccountClientImpl.h	2016-03-16 12:08:30 +0000
@@ -59,6 +59,7 @@
      OnlineAccountClientImpl(std::string const& service_name,
                              std::string const& service_type,
                              std::string const& provider_name,
++                            VariantMap const& auth_params,
                              OnlineAccountClient::MainLoopSelect main_loop_select);
      ~OnlineAccountClientImpl();
@@ -87,6 +88,7 @@
      std::shared_ptr<AgManager> manager();
      std::string service_name();
++    VariantMap const& auth_params() const { return auth_params_; }
      std::shared_ptr<GMainContext> main_loop_context();
      void callback(AccountInfo* info, std::string const& error = "");
@@ -99,6 +101,7 @@
      std::string const service_name_;
      std::string const service_type_;
      std::string const provider_name_;
++    VariantMap const auth_params_;
      OnlineAccountClient::MainLoopSelect const main_loop_select_;
      std::mutex callback_mutex_;
 === modified file 'src/scopes/OnlineAccountClient.cpp'
 --- src/scopes/OnlineAccountClient.cpp	2014-09-12 04:42:19 +0000
 +++ src/scopes/OnlineAccountClient.cpp	2016-03-16 12:08:30 +0000
@@ -29,7 +29,16 @@
                                           std::string const& service_type,
                                           std::string const& provider_name,
                                           MainLoopSelect main_loop_select)
--    : p(new internal::OnlineAccountClientImpl(service_name, service_type, provider_name, main_loop_select))
++    : p(new internal::OnlineAccountClientImpl(service_name, service_type, provider_name, VariantMap(), main_loop_select))
++{
++}
++
++OnlineAccountClient::OnlineAccountClient(std::string const& service_name,
++                                         std::string const& service_type,
++                                         std::string const& provider_name,
++                                         VariantMap const& auth_params,
++                                         MainLoopSelect main_loop_select)
++    : p(new internal::OnlineAccountClientImpl(service_name, service_type, provider_name, auth_params, main_loop_select))
+ {
+ }
 === modified file 'src/scopes/internal/OnlineAccountClientImpl.cpp'
 --- src/scopes/internal/OnlineAccountClientImpl.cpp	2015-11-25 08:55:19 +0000
 +++ src/scopes/internal/OnlineAccountClientImpl.cpp	2016-03-16 12:08:30 +0000
@@ -87,8 +87,8 @@
      service_status.service_authenticated = access_token ? info->service_enabled : false;
      service_status.client_id = client_id ? client_id : "";
      service_status.client_secret = client_secret ? client_secret : "";
--    service_status.access_token = access_token ? access_token : "";
--    service_status.token_secret = token_secret ? token_secret : "";
++    service_status.access_token = (info->service_enabled && access_token) ? access_token : "";
++    service_status.token_secret = (info->service_enabled && token_secret) ? token_secret : "";
      service_status.error = error;
      return service_status;
@@ -131,6 +131,19 @@
      info->account_client->callback(info, error ? error->message : "");
+ }
++static GVariant *variant_to_gvariant(const Variant &v)
++{
++    switch (v.which()) {
++    case Variant::Int: return g_variant_new_int32(v.get_int());
++    case Variant::Bool: return g_variant_new_boolean(v.get_bool());
++    case Variant::String: return g_variant_new_string(v.get_string().c_str());
++    case Variant::Double: return g_variant_new_double(v.get_double());
++    default:
++        std::cerr << "variant_to_gvariant(): unsupported type " << v.serialize_json();
++        return nullptr;
++    }
++}
++
  static void service_update_cb(AgAccountService* account_service, gboolean enabled, AccountInfo* info)
+ {
      std::unique_lock<std::mutex> info_lock(info->mutex);
@@ -179,6 +192,13 @@
                                    g_variant_new_int32(SIGNON_POLICY_NO_USER_INTERACTION));
+         }
++        for (const auto &param: info->account_client->auth_params())
++        {
++            GVariant* value = variant_to_gvariant(param.second);
++            if (!value) continue;
++            g_variant_builder_add(&builder, "{sv}", param.first.c_str(), value);
++        }
++
          info->auth_params.reset(
              g_variant_ref_sink(ag_auth_data_get_login_parameters(auth_data.get(), g_variant_builder_end(&builder))), free_variant);
@@ -249,10 +269,12 @@
  OnlineAccountClientImpl::OnlineAccountClientImpl(std::string const& service_name,
                                                   std::string const& service_type,
                                                   std::string const& provider_name,
++                                                 VariantMap const& auth_params,
                                                   OnlineAccountClient::MainLoopSelect main_loop_select)
      : service_name_(service_name)
      , service_type_(service_type)
      , provider_name_(provider_name)
++    , auth_params_(auth_params)
      , main_loop_select_(main_loop_select)
      , main_loop_is_running_(main_loop_select != OnlineAccountClient::CreateInternalMainLoop)
+ {
@@ -458,6 +480,7 @@
      account_details_map["service_name"] = service_name_;
      account_details_map["service_type"] = service_type_;
      account_details_map["provider_name"] = provider_name_;
++    account_details_map["auth_params"] = auth_params_;
      account_details_map["login_passed_action"] = static_cast<int>(login_passed_action);
      account_details_map["login_failed_action"] = static_cast<int>(login_failed_action);
@@ -473,6 +496,7 @@
      account_details_map["service_name"] = service_name_;
      account_details_map["service_type"] = service_type_;
      account_details_map["provider_name"] = provider_name_;
++    account_details_map["auth_params"] = auth_params_;
      account_details_map["login_passed_action"] = static_cast<int>(login_passed_action);
      account_details_map["login_failed_action"] = static_cast<int>(login_failed_action);
 === modified file 'test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp'
 --- test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp	2015-04-15 10:05:42 +0000
 +++ test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp	2016-03-16 12:08:30 +0000
@@ -58,7 +58,13 @@
          setenv("AG_SERVICE_TYPES", TEST_DATA_DIR, false);
          setenv("AG_PROVIDERS", TEST_DATA_DIR, false);
--        oa_client_.reset(new OnlineAccountClient("TestService", "sharing", "TestProvider", main_loop_select));
++        VariantMap params;
++        params["ClientId"] = "abcXYZ";
++        params["Timeout"] = 200;
++        params["UseSSL"] = true;
++        params["Humidity"] = 0.3;
++        params["Unsupported"] = VariantMap();
++        oa_client_.reset(new OnlineAccountClient("TestService", "sharing", "TestProvider", params, main_loop_select));
          manager_ = oa_client_->p->manager();
          main_loop_context_ = oa_client_->p->main_loop_context();
@@ -295,13 +301,61 @@
          : OnlineAccountClientTest(OnlineAccountClient::RunInExternalMainLoop) {}
  };
++class SignondMock
++{
++public:
++    SignondMock()
++        : mock_(dbus_test_dbus_mock_new("com.google.code.AccountsSSO.SingleSignOn"))
++    {
++        DbusTestDbusMockObject* authService =
++            dbus_test_dbus_mock_get_object(mock_,
++                                           "/com/google/code/AccountsSSO/SingleSignOn",
++                                           "com.google.code.AccountsSSO.SingleSignOn.AuthService",
++                                           NULL);
++
++        dbus_test_dbus_mock_object_add_method(mock_, authService,
++                                              "getAuthSessionObjectPath",
++                                              G_VARIANT_TYPE("(us)"),
++                                              G_VARIANT_TYPE("s"),
++                                              "ret = '/AuthSession'",
++                                              NULL);
++
++        DbusTestDbusMockObject* authSession =
++            dbus_test_dbus_mock_get_object(mock_,
++                                           "/AuthSession",
++                                           "com.google.code.AccountsSSO.SingleSignOn.AuthSession",
++                                           NULL);
++
++        static const char* const process_code =
++            "data = dict(args[0])\n"
++            "if 'ClientId' in data:\n"
++            "    data['AccessToken'] = data['ClientId'].swapcase()\n"
++            "ret = data\n";
++        dbus_test_dbus_mock_object_add_method(mock_, authSession,
++                                              "process",
++                                              G_VARIANT_TYPE("(a{sv}s)"),
++                                              G_VARIANT_TYPE("a{sv}"),
++                                              process_code,
++                                              NULL);
++    }
++
++    DbusTestTask* task() const { return DBUS_TEST_TASK(mock_); }
++
++private:
++    DbusTestDbusMock* mock_;
++};
++
  } // namespace testing
  } // namespace scopes
  } // namespace unity
  TEST_F(OnlineAccountClientTest, register_account_login_result)
+ {
--    OnlineAccountClient oa_client("test_service_name", "test_service_type", "test_provider");
++    VariantMap params;
++    params["ClientId"] = "abc";
++    params["Timeout"] = 200;
++    params["UseSSL"] = true;
++    OnlineAccountClient oa_client("test_service_name", "test_service_type", "test_provider", params);
      CategoryRegistry reg;
      auto cat = reg.register_category("1", "title", "icon", nullptr, CategoryRenderer());
@@ -319,6 +373,7 @@
      EXPECT_NE(details.end(), details.find("service_name"));
      EXPECT_NE(details.end(), details.find("service_type"));
      EXPECT_NE(details.end(), details.find("provider_name"));
++    EXPECT_NE(details.end(), details.find("auth_params"));
      EXPECT_NE(details.end(), details.find("login_passed_action"));
      EXPECT_NE(details.end(), details.find("login_failed_action"));
@@ -326,6 +381,10 @@
      EXPECT_EQ("test_service_name", details.at("service_name").get_string());
      EXPECT_EQ("test_service_type", details.at("service_type").get_string());
      EXPECT_EQ("test_provider", details.at("provider_name").get_string());
++    VariantMap actual_params = details.at("auth_params").get_dict();
++    EXPECT_EQ("abc", actual_params.at("ClientId").get_string());
++    EXPECT_EQ(200, actual_params.at("Timeout").get_int());
++    EXPECT_EQ(true, actual_params.at("UseSSL").get_bool());
      EXPECT_EQ(OnlineAccountClient::InvalidateResults, static_cast<OnlineAccountClient::PostLoginAction>(details.at("login_passed_action").get_int()));
      EXPECT_EQ(OnlineAccountClient::DoNothing, static_cast<OnlineAccountClient::PostLoginAction>(details.at("login_failed_action").get_int()));
+ }
@@ -423,18 +482,18 @@
      OnlineAccountClient::ServiceStatus enabled_status;
      enabled_status.account_id = 1;
      enabled_status.service_enabled = true;
--    enabled_status.service_authenticated = false;
--    enabled_status.client_id = "69842936499-sdflkbhslufhgrjamwlicefhb.apps.test.com";
++    enabled_status.service_authenticated = true;
++    enabled_status.client_id = "abcXYZ";
      enabled_status.client_secret = "lj3i8iorep0w03994jwjef0j";
--    enabled_status.access_token = "";
++    enabled_status.access_token = "ABCxyz";
      enabled_status.token_secret = "";
--    enabled_status.error = "GDBus.Error:com.google.code.AccountsSSO.SingleSignOn.Error.MethodNotKnown: Authentication method is not known.";
++    enabled_status.error = "";
      OnlineAccountClient::ServiceStatus disabled_status;
      disabled_status.account_id = 1;
      disabled_status.service_enabled = false;
      disabled_status.service_authenticated = false;
--    disabled_status.client_id = "69842936499-sdflkbhslufhgrjamwlicefhb.apps.test.com";
++    disabled_status.client_id = "abcXYZ";
      disabled_status.client_secret = "lj3i8iorep0w03994jwjef0j";
      disabled_status.access_token = "";
      disabled_status.token_secret = "";
@@ -505,7 +564,11 @@
+ {
      std::shared_ptr<DbusTestService> dbus_test_service;
      dbus_test_service.reset(dbus_test_service_new(nullptr), g_object_unref);
--    dbus_test_service_run(dbus_test_service.get());
++
++    unity::scopes::testing::SignondMock signondMock;
++    dbus_test_service_add_task(dbus_test_service.get(), signondMock.task());
++
++    dbus_test_service_start_tasks(dbus_test_service.get());
      ::testing::InitGoogleTest(&argc, argv);
      return RUN_ALL_TESTS();