Merge lp:~mardy/click-reviewers-tools/new-account-hook into lp:click-reviewers-tools

[Alberto Mardegan]

Support the new "accounts" hook

A new hook for Online Accounts is being added, and it's intended to replace both the "account-application" and "account-service" hooks. It takes a JSON file, which acts as a manifest for how the app uses OA, and the hook program will then generate the needed .application and .service files.

With this change we check that the new "accounts" hook becomes the mandatory OA hook starting from the 15.10 framework.

Support the new "accounts" hook

A new hook for Online Accounts is being added, and it's intended to replace both the "account-application" and "account-service" hooks. It takes a JSON file, which acts as a manifest for how the app uses OA, and the hook program will then generate the needed .application and .service files.

With this change we check that the new "accounts" hook becomes the mandatory OA hook starting from the 15.10 framework.

[lp:~mardy/click-reviewers-tools/new-account-hook]

511. By Alberto Mardegan on 2015-08-21

OA: the name and description keys should not be mandatory

512. By Alberto Mardegan on 2015-08-24

Support plugins in new OA hook

513. By Alberto Mardegan on 2015-08-26

Add "auth" key to plugins

[Jamie Strandboge (jdstrand)]

Thanks for the MP! I've got a few comments inline.

review: Needs Fixing

[lp:~mardy/click-reviewers-tools/new-account-hook]

514. By Alberto Mardegan on 2015-09-10

Address review comments, turn 15.10 errors into warnings

[Jamie Strandboge (jdstrand)]

review: Approve

=== modified file 'clickreviews/cr_common.py'
--- clickreviews/cr_common.py	2015-08-20 20:54:51 +0000
+++ clickreviews/cr_common.py	2015-09-10 08:33:02 +0000
@@ -66,6 +66,7 @@
     # click service)
     app_allowed_peer_hooks = ["account-application",
                               "account-service",
+                              "accounts",
                               "account-provider",
                               "account-qml-plugin",
                               "apparmor",
@@ -76,6 +77,7 @@
                               ]
     scope_allowed_peer_hooks = ["account-application",
                                 "account-service",
+                                "accounts",
                                 "apparmor",
                                 "scope",
                                 ]

=== modified file 'clickreviews/cr_online_accounts.py'
--- clickreviews/cr_online_accounts.py	2015-08-13 21:07:13 +0000
+++ clickreviews/cr_online_accounts.py	2015-09-10 08:33:02 +0000
@@ -16,8 +16,10 @@
 
 from __future__ import print_function
 
-from clickreviews.cr_common import ClickReview, error
+from clickreviews.cr_common import ClickReview, error, open_file_read
+import json
 import os
+import re
 # http://lxml.de/tutorial.html
 import lxml.etree as etree
 
@@ -40,6 +42,13 @@
             ClickReview.app_allowed_peer_hooks + \
             ClickReview.scope_allowed_peer_hooks
 
+        peer_hooks['accounts'] = dict()
+        peer_hooks['accounts']['allowed'] = \
+            [h for h in (ClickReview.app_allowed_peer_hooks +
+                         ClickReview.scope_allowed_peer_hooks)
+             if not h.startswith('account-')]
+        peer_hooks['accounts']['required'] = ['apparmor']
+
         peer_hooks['account-provider'] = dict()
         peer_hooks['account-provider']['required'] = ['account-qml-plugin',
                                                       'apparmor'
@@ -64,7 +73,8 @@
         self.accounts_files = dict()
         self.accounts = dict()
 
-        self.account_hooks = ['account-application',
+        self.account_hooks = ['accounts',
+                              'account-application',
                               'account-provider',
                               'account-qml-plugin',
                               'account-service']
@@ -77,7 +87,7 @@
                     error("manifest malformed: hooks/%s/%s is not a str" % (
                           app, h))
 
-                (full_fn, xml) = self._extract_account(app, h)
+                (full_fn, parsed) = self._extract_account(app, h)
 
                 if app not in self.accounts_files:
                     self.accounts_files[app] = dict()
@@ -85,7 +95,28 @@
 
                 if app not in self.accounts:
                     self.accounts[app] = dict()
-                self.accounts[app][h] = xml
+                self.accounts[app][h] = parsed
+
+        self.required_keys = dict()
+        self.allowed_keys = dict()
+        self.required_keys["service"] = [
+            ('provider', str),
+        ]
+        self.allowed_keys["service"] = [
+            ('auth', dict),
+            ('name', str),
+            ('description', str),
+        ]
+        self.required_keys["plugin"] = [
+            ('provider', str),
+            ('name', str),
+            ('icon', str),
+            ('qml', str),
+        ]
+        self.allowed_keys["plugin"] = [
+            ('auth', dict),
+        ]
+        self.provider_re = re.compile('^[a-zA-Z0-9_.-]+$')
 
     def _extract_account(self, app, account_type):
         '''Extract accounts'''
@@ -100,6 +131,23 @@
         # the hook present for now
         if account_type == "account-qml-plugin":
             return (fn, True)
+        elif account_type == "accounts":
+            fh = open_file_read(fn)
+            contents = ""
+            for line in fh.readlines():
+                contents += line
+            fh.close()
+
+            try:
+                jd = json.loads(contents)
+            except Exception as e:
+                error("accounts json unparseable: %s (%s):\n%s" % (bn,
+                      str(e), contents))
+
+            if not isinstance(jd, dict):
+                error("accounts json is malformed: %s:\n%s" % (bn, contents))
+
+            return (fn, jd)
         else:
             try:
                 tree = etree.parse(fn)
@@ -108,6 +156,113 @@
                 error("accounts xml unparseable: %s (%s)" % (bn, str(e)))
             return (fn, xml)
 
+    def check_hooks_versions(self):
+        '''Check hooks versions'''
+        framework = self.manifest['framework']
+        if not framework.startswith("ubuntu-sdk"):
+            return
+        t = "error"
+        if framework < "ubuntu-sdk-15.10":
+            for app in sorted(self.accounts.keys()):
+                for hook in self.accounts[app].keys():
+                    if hook == "accounts":
+                        n = self._get_check_name('%s_hook' % hook, app=app)
+                        s = "'accounts' hook is not available in '%s' (must be 15.10 or later)" % \
+                            (framework)
+                        self._add_result(t, n, s)
+            return
+        hook_state = "disallowed"
+        if framework < "ubuntu-sdk-16.04":
+            t = "warn"
+            hook_state = "deprecated"
+        for app in sorted(self.accounts.keys()):
+            for hook in self.accounts[app].keys():
+                if hook.startswith("account-"):
+                    n = self._get_check_name('%s_hook' % hook, app=app)
+                    s = "'%s' is %s in %s: use 'accounts' hook instead" % \
+                        (hook, hook_state, framework)
+                    self._add_result(t, n, s)
+
+    def _check_object(self, obj_type, obj, n):
+        t = "info"
+        s = "OK"
+        if not isinstance(obj, dict):
+            t = "error"
+            s = "%s is not an object" % obj_type
+            self._add_result(t, n, s)
+            return
+
+        for (k, vt) in self.required_keys[obj_type]:
+            if k not in obj.keys():
+                t = "error"
+                s = "required key '%s' is missing" % k
+                self._add_result(t, n, s)
+        if t == "error":
+            return
+
+        known_keys = self.required_keys[obj_type] + self.allowed_keys[obj_type]
+        for (k, v) in obj.items():
+            type_list = [kk[1] for kk in known_keys if kk[0] == k]
+            if len(type_list) < 1:
+                t = "error"
+                s = "unrecognized key '%s'" % k
+                self._add_result(t, n, s)
+                continue
+            if not isinstance(v, type_list[0]):
+                t = "error"
+                s = "value for '%s' must be of type %s" % (k, type_list[0])
+                self._add_result(t, n, s)
+                continue
+            if k == 'provider' and not self.provider_re.match(v):
+                t = "error"
+                s = "'provider' must only consist of alphanumeric characters"
+                self._add_result(t, n, s)
+        self._add_result(t, n, s)
+
+    def _check_object_list(self, app, key, obj_type, obj_list):
+        t = 'info'
+        n = self._get_check_name('accounts_%s' % key, app=app)
+        if not isinstance(obj_list, list):
+            t = "error"
+            s = "'%s' is not a list" % key
+        elif len(obj_list) < 1:
+            t = "error"
+            s = "'%s' is empty" % key
+        if t == "error":
+            self._add_result(t, n, s)
+            return
+
+        for (i, obj) in enumerate(obj_list):
+            n = self._get_check_name('accounts_%s' % (obj_type), app=app, extra=str(i))
+            self._check_object(obj_type, obj, n)
+
+    def check_manifest(self):
+        '''Check manifest'''
+        for app in sorted(self.accounts.keys()):
+            account_type = "accounts"
+
+            t = 'info'
+            n = self._get_check_name('%s_root' % account_type, app=app)
+            s = "OK"
+            if account_type not in self.accounts[app]:
+                s = "OK (missing)"
+                self._add_result(t, n, s)
+                continue
+
+            n = self._get_check_name('%s_services' % account_type, app=app)
+            if 'services' not in self.accounts[app][account_type]:
+                t = "error"
+                s = "'services' key is missing"
+                self._add_result(t, n, s)
+                continue
+            services = self.accounts[app][account_type]['services']
+            self._check_object_list(app, "services", "service", services)
+
+            n = self._get_check_name('%s_plugins' % account_type, app=app)
+            if 'plugins' in self.accounts[app][account_type]:
+                plugins = self.accounts[app][account_type]['plugins']
+                self._check_object_list(app, "plugins", "plugin", plugins)
+
     def check_application(self):
         '''Check application'''
         for app in sorted(self.accounts.keys()):

=== modified file 'clickreviews/cr_tests.py'
--- clickreviews/cr_tests.py	2015-07-07 19:42:41 +0000
+++ clickreviews/cr_tests.py	2015-09-10 08:33:02 +0000
@@ -40,6 +40,7 @@
 TEST_URLS = dict()
 TEST_SCOPES = dict()
 TEST_CONTENT_HUB = dict()
+TEST_ACCOUNTS_MANIFEST = dict()
 TEST_ACCOUNTS_APPLICATION = dict()
 TEST_ACCOUNTS_PROVIDER = dict()
 TEST_ACCOUNTS_QML_PLUGIN = dict()
@@ -190,7 +191,10 @@
     '''Pretend we read the accounts file'''
     f = app
     val = None
-    if account_type == "account-application":
+    if account_type == "accounts":
+        f += ".accounts"
+        val = TEST_ACCOUNTS_MANIFEST[app]
+    elif account_type == "account-application":
         f += ".application"
         val = TEST_ACCOUNTS_APPLICATION[app]
     elif account_type == "account-provider":
@@ -449,6 +453,7 @@
         self.test_url_dispatcher = dict()
         self.test_scopes = dict()
         self.test_content_hub = dict()
+        self.test_accounts_manifest = dict()
         self.test_accounts_application = dict()
         self.test_accounts_provider = dict()
         self.test_accounts_qml_plugin = dict()
@@ -488,6 +493,7 @@
             self.set_test_content_hub(app, None, None)
 
             # Reset to no accounts entries in manifest
+            self.set_test_account(app, "accounts", None)
             self.set_test_account(app, "account-application", None)
             self.set_test_account(app, "account-provider", None)
             self.set_test_account(app, "account-qml-plugin", None)
@@ -515,6 +521,7 @@
         self._update_test_url_dispatcher()
         self._update_test_scopes()
         self._update_test_content_hub()
+        self._update_test_accounts_manifest()
         self._update_test_accounts_application()
         self._update_test_accounts_provider()
         self._update_test_accounts_qml_plugin()
@@ -618,6 +625,15 @@
                 "%s.content.json" % app
         self._update_test_manifest()
 
+    def _update_test_accounts_manifest(self):
+        global TEST_ACCOUNTS_MANIFEST
+        TEST_ACCOUNTS_MANIFEST = dict()
+        for app in self.test_accounts_manifest.keys():
+            TEST_ACCOUNTS_MANIFEST[app] = self.test_accounts_manifest[app]
+            self.test_manifest["hooks"][app]["accounts"] = \
+                "%s.accounts" % app
+        self._update_test_manifest()
+
     def _update_test_accounts_application(self):
         global TEST_ACCOUNTS_APPLICATION
         TEST_ACCOUNTS_APPLICATION = dict()
@@ -923,7 +939,9 @@
 
     def set_test_account(self, app, account_type, value):
         '''Set accounts XML. If value is None, remove from manifest'''
-        if account_type == "account-application":
+        if account_type == "accounts":
+            d = self.test_accounts_manifest
+        elif account_type == "account-application":
             d = self.test_accounts_application
         elif account_type == "account-provider":
             d = self.test_accounts_provider
@@ -940,7 +958,9 @@
         else:
             d[app] = value
 
-        if account_type == "account-application":
+        if account_type == "accounts":
+            self._update_test_accounts_manifest()
+        elif account_type == "account-application":
             self._update_test_accounts_application()
         elif account_type == "account-provider":
             self._update_test_accounts_provider()
@@ -1115,6 +1135,8 @@
         TEST_SCOPES = dict()
         global TEST_CONTENT_HUB
         TEST_CONTENT_HUB = dict()
+        global TEST_ACCOUNTS_MANIFEST
+        TEST_ACCOUNTS_MANIFEST = dict()
         global TEST_ACCOUNTS_APPLICATION
         TEST_ACCOUNTS_APPLICATION = dict()
         global TEST_ACCOUNTS_PROVIDER
@@ -1122,7 +1144,7 @@
         global TEST_ACCOUNTS_QML_PLUGIN
         TEST_ACCOUNTS_QML_PLUGIN = dict()
         global TEST_ACCOUNTS_SERVICE
-        TEST_ACCOUNTS_APPLICATION = dict()
+        TEST_ACCOUNTS_SERVICE = dict()
         global TEST_PUSH_HELPER
         TEST_PUSH_HELPER = dict()
         global TEST_BIN_PATH

=== modified file 'clickreviews/tests/test_cr_online_accounts.py'
--- clickreviews/tests/test_cr_online_accounts.py	2015-07-22 16:30:36 +0000
+++ clickreviews/tests/test_cr_online_accounts.py	2015-09-10 08:33:02 +0000
@@ -16,6 +16,7 @@
 
 from clickreviews.cr_online_accounts import ClickReviewAccounts
 import clickreviews.cr_tests as cr_tests
+import json
 import lxml.etree as etree
 
 
@@ -84,6 +85,185 @@
             # More can go here, see /usr/share/accounts/providers/*
         return xml
 
+    def test_check_hooks_versions_new(self):
+        '''Test check_hooks_versions() - new hook'''
+        self.set_test_manifest("framework", "ubuntu-sdk-15.10")
+        self.set_test_account(self.default_appname, "accounts", dict())
+        c = ClickReviewAccounts(self.test_name)
+        c.check_hooks_versions()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_hooks_versions_deprecated_service(self):
+        '''Test check_hooks_versions() - deprecated -service hook'''
+        self.set_test_manifest("framework", "ubuntu-sdk-15.10")
+        self.set_test_account(self.default_appname, "account-service", dict())
+        c = ClickReviewAccounts(self.test_name)
+        c.check_hooks_versions()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 1, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_hooks_versions_disallowed_service(self):
+        '''Test check_hooks_versions() - deprecated -service hook'''
+        self.set_test_manifest("framework", "ubuntu-sdk-16.04")
+        self.set_test_account(self.default_appname, "account-service", dict())
+        c = ClickReviewAccounts(self.test_name)
+        c.check_hooks_versions()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_hooks_versions_deprecated_application(self):
+        '''Test check_hooks_versions() - deprecated -application hook'''
+        self.set_test_manifest("framework", "ubuntu-sdk-15.10")
+        self.set_test_account(self.default_appname, "account-application", dict())
+        c = ClickReviewAccounts(self.test_name)
+        c.check_hooks_versions()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 1, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_hooks_versions_old_framework(self):
+        '''Test check_hooks_versions() - deprecated -application hook'''
+        self.set_test_manifest("framework", "ubuntu-sdk-15.04")
+        self.set_test_account(self.default_appname, "account-application", dict())
+        self.set_test_account(self.default_appname, "account-service", dict())
+        c = ClickReviewAccounts(self.test_name)
+        c.check_hooks_versions()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest(self):
+        '''Test check_manifest()'''
+        data = json.loads('''{
+          "translations": "my-app",
+          "services": [
+            {
+              "name": "Example",
+              "provider": "myapp.com_example",
+              "description": "publish my photos in example.com",
+              "auth": {
+                "oauth2/web_server": {
+                  "ClientId": "foo",
+                  "ClientSecret": "bar",
+                  "UseSSL": false,
+                  "Scopes": ["one scope","and another"]
+                }
+              }
+            },
+            {
+              "provider": "becool"
+            }
+          ],
+          "plugins": [
+            {
+              "provider": "example",
+              "name": "Example site",
+              "icon": "example.png",
+              "qml": "qml_files"
+            }
+          ]
+        }''')
+        self.set_test_account(self.default_appname, "accounts", data)
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 3, 'warn': 0, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest_not_specified(self):
+        '''Test check_manifest() - not specified'''
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 0}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest_missing_services(self):
+        '''Test check_manifest() - missing services'''
+        data = json.loads('''{ "translations": "my-app" }''')
+        self.set_test_account(self.default_appname, "accounts", data)
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest_invalid_services(self):
+        '''Test check_manifest() - invalid services'''
+        data = json.loads('''{ "services": 12 }''')
+        self.set_test_account(self.default_appname, "accounts", data)
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest_empty_services(self):
+        '''Test check_manifest() - empty services'''
+        data = json.loads('''{ "services": [] }''')
+        self.set_test_account(self.default_appname, "accounts", data)
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest_empty_service(self):
+        '''Test check_manifest() - empty services'''
+        data = json.loads('''{ "services": [{}] }''')
+        self.set_test_account(self.default_appname, "accounts", data)
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest_no_provider(self):
+        '''Test check_manifest() - no provider'''
+        data = json.loads('''{ "services": [{
+          "name": "Example",
+          "description": "Hello world"
+        }] }''')
+        self.set_test_account(self.default_appname, "accounts", data)
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest_invalid_provider(self):
+        '''Test check_manifest() - invalid provider'''
+        data = json.loads('''{ "services": [{
+          "name": "Example",
+          "provider": "no/slashes.please",
+          "description": "Hello world"
+        }] }''')
+        self.set_test_account(self.default_appname, "accounts", data)
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
+    def test_check_manifest_unknown_key(self):
+        '''Test check_manifest() - unknown key'''
+        data = json.loads('''{ "services": [{
+          "name": "Example",
+          "provider": "example",
+          "description": "Hello world",
+          "intruder": "Who, me?"
+        }] }''')
+        self.set_test_account(self.default_appname, "accounts", data)
+        c = ClickReviewAccounts(self.test_name)
+        c.check_manifest()
+        r = c.click_report
+        expected_counts = {'info': 0, 'warn': 0, 'error': 1}
+        self.check_results(r, expected_counts)
+
     def test_check_application(self):
         '''Test check_application()'''
         xml = self._stub_application()