diff -Nru click-reviewers-tools-0.10/bin/click-review click-reviewers-tools-0.13/bin/click-review --- click-reviewers-tools-0.10/bin/click-review 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/click-review 2014-10-02 15:16:19.000000000 +0000 @@ -91,6 +91,7 @@ if review: review.do_checks() self.results[section] = review.click_report + return section return None def run_all_checks(self): diff -Nru click-reviewers-tools-0.10/bin/clickreviews/apparmor_policy.py click-reviewers-tools-0.13/bin/clickreviews/apparmor_policy.py --- click-reviewers-tools-0.10/bin/clickreviews/apparmor_policy.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/apparmor_policy.py 2014-10-02 15:16:19.000000000 +0000 @@ -21,7 +21,7 @@ # XXX: This is a hack and will be gone, as soon as myapps has an API for this. AA_POLICY_DATA_URL = \ - "http://bazaar.launchpad.net/~click-reviewers/click-reviewers-tools/trunk/view/head:/data/apparmor-easyprof-ubuntu.json" + "http://bazaar.launchpad.net/~click-reviewers/click-reviewers-tools/trunk/download/head:/apparmoreasyprofubun-20140711222314-oeohtxzvf9a58fa6-1/apparmor-easyprof-ubuntu.json" def get_policy_file(fn): diff -Nru click-reviewers-tools-0.10/bin/clickreviews/cr_common.py click-reviewers-tools-0.13/bin/clickreviews/cr_common.py --- click-reviewers-tools-0.10/bin/clickreviews/cr_common.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/cr_common.py 2014-10-02 15:16:19.000000000 +0000 @@ -30,15 +30,18 @@ import types DEBUGGING = False -UNPACK_DIR = None +UNPACK_DIRS = [] # cleanup import atexit def cleanup_unpack(): - if UNPACK_DIR is not None and os.path.isdir(UNPACK_DIR): - recursive_rm(UNPACK_DIR) + global UNPACK_DIRS + if len(UNPACK_DIRS) > 0: + for d in UNPACK_DIRS: + if d is not None and os.path.isdir(d): + recursive_rm(d) atexit.register(cleanup_unpack) @@ -76,8 +79,8 @@ self.click_report_output = "json" self.unpack_dir = unpack_click(fn) - global UNPACK_DIR - UNPACK_DIR = self.unpack_dir + global UNPACK_DIRS + UNPACK_DIRS.append(self.unpack_dir) # Get some basic information from the control file diff -Nru click-reviewers-tools-0.10/bin/clickreviews/cr_desktop.py click-reviewers-tools-0.13/bin/clickreviews/cr_desktop.py --- click-reviewers-tools-0.10/bin/clickreviews/cr_desktop.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/cr_desktop.py 2014-10-02 15:16:19.000000000 +0000 @@ -46,6 +46,14 @@ elif 'pay-ui' in self.manifest['hooks'][app]: # msg("Skipped missing desktop hook for pay-ui '%s'" % app) continue + elif 'account-provider' in self.manifest['hooks'][app]: + # msg("Skipped missing desktop hook for account-provider" + # " '%s'" % app) + continue + elif 'account-qml-plugin' in self.manifest['hooks'][app]: + # msg("Skipped missing desktop hook for account-qml-plugin" + # " '%s'" % app) + continue else: error("could not find desktop hook for '%s'" % app) if not isinstance(self.manifest['hooks'][app]['desktop'], str): diff -Nru click-reviewers-tools-0.10/bin/clickreviews/cr_lint.py click-reviewers-tools-0.13/bin/clickreviews/cr_lint.py --- click-reviewers-tools-0.10/bin/clickreviews/cr_lint.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/cr_lint.py 2014-10-02 15:16:19.000000000 +0000 @@ -342,6 +342,9 @@ mutually_exclusive = ['scope', 'desktop'] for app in self.manifest['hooks']: + t = 'info' + n = 'exclusive_hooks_%s' % (app) + s = "OK" found = [] for i in mutually_exclusive: if i in self.manifest['hooks'][app]: @@ -351,6 +354,17 @@ s = "'%s' hooks should not be used together" % ", ".join(found) self._add_result(t, n, s) + for app in self.manifest['hooks']: + if "apparmor" in self.manifest['hooks'][app]: + t = 'info' + n = 'sdk_security_extension_%s' % (app) + s = "OK" + fn = self.manifest['hooks'][app]['apparmor'] + if not fn.endswith(".apparmor"): + t = 'info' + s = '%s does not end with .apparmor (ok if not using sdk)' % fn + self._add_result(t, n, s) + def check_hooks_unknown(self): '''Check if have any unknown hooks''' t = 'info' @@ -652,14 +666,19 @@ for k in sorted(self.manifest): if k.startswith('x-'): found.append(k) + + # Some local extensions are ok for core apps, so just skip them + if self.is_core_app: + for i in ['x-source', 'x-test']: + if i in found: + found.remove(i) + if len(found) > 0: t = 'warn' plural = "" if len(found) > 1: plural = "s" s = 'found unofficial extension%s: %s' % (plural, ', '.join(found)) - if 'x-source' in k and self.is_core_app: - s += ' (x-source found, but app is a core app, which is fine)' self._add_result(t, n, s) def check_package_filename(self): diff -Nru click-reviewers-tools-0.10/bin/clickreviews/cr_online_accounts.py click-reviewers-tools-0.13/bin/clickreviews/cr_online_accounts.py --- click-reviewers-tools-0.10/bin/clickreviews/cr_online_accounts.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/cr_online_accounts.py 2014-10-02 15:16:19.000000000 +0000 @@ -88,6 +88,29 @@ self._add_result(t, n, s) continue + # account-application must always appear with apparmor + t = 'info' + n = '%s_%s_apparmor' % (app, account_type) + s = "OK" + if 'apparmor' not in self.manifest['hooks'][app]: + t = 'error' + s = "missing 'apparmor' entry" + self._add_result(t, n, s) + + # account-application must always appear with desktop or scope + t = 'info' + n = '%s_%s_desktop_or_scope' % (app, account_type) + s = "OK" + found = False + for k in ['desktop', 'scope']: + if k in self.manifest['hooks'][app]: + found = True + break + if not found: + t = 'error' + s = "missing 'desktop' or 'scope' entry" + self._add_result(t, n, s) + root_tag = self.accounts[app][account_type].tag.lower() if root_tag != "application": t = 'error' @@ -141,6 +164,24 @@ self._add_result(t, n, s) continue + # account service must always appear with account-application + t = 'info' + n = '%s_%s_account-application' % (app, account_type) + s = "OK" + if 'account-application' not in self.accounts[app]: + t = 'error' + s = "missing 'account-application' entry" + self._add_result(t, n, s) + + # account-service must always appear with apparmor + t = 'info' + n = '%s_%s_apparmor' % (app, account_type) + s = "OK" + if 'apparmor' not in self.manifest['hooks'][app]: + t = 'error' + s = "missing 'apparmor' entry" + self._add_result(t, n, s) + root_tag = self.accounts[app][account_type].tag.lower() if root_tag != "service": t = 'error' @@ -192,6 +233,40 @@ manual_review = True self._add_result(t, n, s, manual_review=manual_review) + # account-provider must always appear with apparmor + t = 'info' + n = '%s_%s_apparmor' % (app, account_type) + s = "OK" + if 'apparmor' not in self.manifest['hooks'][app]: + t = 'error' + s = "missing 'apparmor' entry" + self._add_result(t, n, s) + + # account-provider must always appear with account-qml-plugin + t = 'info' + n = '%s_%s_account-qml-plugin' % (app, account_type) + s = "OK" + if 'account-qml-plugin' not in self.accounts[app]: + t = 'error' + s = "missing 'account-qml-plugin' entry" + self._add_result(t, n, s) + + # account-provider must never appear with account-application or + # account-service + t = 'info' + n = '%s_%s_account-application_or_account-service' % (app, + account_type) + s = "OK" + found = False + for i in ['account-application', 'account-service']: + if i in self.accounts[app]: + found = True + if found: + t = 'error' + s = "must not specify account-application or account-service" \ + " with %s" % account_type + self._add_result(t, n, s) + t = 'info' n = '%s_%s_root' % (app, account_type) s = "OK" @@ -233,3 +308,37 @@ s = "(MANUAL REVIEW) '%s' not allowed" % account_type manual_review = True self._add_result(t, n, s, manual_review=manual_review) + + # account-qml-plugin must always appear with apparmor + t = 'info' + n = '%s_%s_apparmor' % (app, account_type) + s = "OK" + if 'apparmor' not in self.manifest['hooks'][app]: + t = 'error' + s = "missing 'apparmor' entry" + self._add_result(t, n, s) + + # account-qml-plugin must always appear with account-provider + t = 'info' + n = '%s_%s_account-provider' % (app, account_type) + s = "OK" + if 'account-provider' not in self.accounts[app]: + t = 'error' + s = "missing 'account-provider' entry" + self._add_result(t, n, s) + + # account-qml-plugin must never appear with account-application or + # account-service + t = 'info' + n = '%s_%s_account-application_or_account-service' % (app, + account_type) + s = "OK" + found = False + for i in ['account-application', 'account-service']: + if i in self.accounts[app]: + found = True + if found: + t = 'error' + s = "must not specify account-application or account-service" \ + " with %s" % account_type + self._add_result(t, n, s) diff -Nru click-reviewers-tools-0.10/bin/clickreviews/cr_security.py click-reviewers-tools-0.13/bin/clickreviews/cr_security.py --- click-reviewers-tools-0.10/bin/clickreviews/cr_security.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/cr_security.py 2014-10-02 15:16:19.000000000 +0000 @@ -16,10 +16,9 @@ from __future__ import print_function -from clickreviews.cr_common import ClickReview, error, warn +from clickreviews.cr_common import ClickReview, error import clickreviews.cr_common as cr_common import clickreviews.apparmor_policy as apparmor_policy -import glob import json import os @@ -69,6 +68,7 @@ 'webview'] self.allowed_push_helper_policy_groups = ['push-notification-client'] + self.allowed_network_scope_policy_groups = ['accounts', 'networking'] self.redflag_templates = ['unconfined'] self.extraneous_templates = ['ubuntu-sdk', @@ -396,6 +396,10 @@ for p in m['policy_groups']: if p not in self.allowed_push_helper_policy_groups: bad.append(p) + elif p == "networking": + # The above covers this, but let's be very explicit and + # never allow networking with push-notification-client + bad.append(p) if len(bad) > 0: t = 'error' s = "found unusual policy groups: %s" % ", ".join(bad) @@ -421,9 +425,8 @@ bad = [] for p in m['policy_groups']: if m['template'] == 'ubuntu-scope-network': - # networking scopes shouldn't have access to anything - # (for now, this may change with trust store (eg, location) - if p != 'networking': + # networking scopes should have extremely limited access + if p not in self.allowed_network_scope_policy_groups: bad.append(p) # jdstrand, 2014-06-05: ubuntu-scope-local-content is no longer available # elif m['template'] == 'ubuntu-scope-local-content': diff -Nru click-reviewers-tools-0.10/bin/clickreviews/cr_tests.py click-reviewers-tools-0.13/bin/clickreviews/cr_tests.py --- click-reviewers-tools-0.10/bin/clickreviews/cr_tests.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/cr_tests.py 2014-10-02 15:16:19.000000000 +0000 @@ -89,7 +89,7 @@ def _get_security_manifest(self, app): '''Pretend we read the security manifest file''' - return ("%s.json" % app, json.loads(TEST_SECURITY[app])) + return ("%s.apparmor" % app, json.loads(TEST_SECURITY[app])) def _get_security_supported_policy_versions(self): @@ -289,7 +289,7 @@ self.default_appname = "test-app" self.test_manifest["hooks"][self.default_appname] = dict() self.test_manifest["hooks"][self.default_appname]["apparmor"] = \ - "%s.json" % self.default_appname + "%s.apparmor" % self.default_appname self.test_manifest["hooks"][self.default_appname]["desktop"] = \ "%s.desktop" % self.default_appname self.test_manifest["hooks"][self.default_appname]["urls"] = \ diff -Nru click-reviewers-tools-0.10/bin/clickreviews/modules.py click-reviewers-tools-0.13/bin/clickreviews/modules.py --- click-reviewers-tools-0.10/bin/clickreviews/modules.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/modules.py 2014-10-02 15:16:19.000000000 +0000 @@ -68,4 +68,9 @@ init_object = find_main_class(module_name) if not init_object: return None - return init_object(click_file) + try: + ob = init_object(click_file) + except TypeError as e: + print('Could not init %s: %s' % (init_object, str(e))) + return None + return ob diff -Nru click-reviewers-tools-0.10/bin/clickreviews/remote.py click-reviewers-tools-0.13/bin/clickreviews/remote.py --- click-reviewers-tools-0.10/bin/clickreviews/remote.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/remote.py 2014-10-02 15:16:19.000000000 +0000 @@ -95,10 +95,16 @@ ''' j = {} if local_copy_fn and os.path.exists(local_copy_fn): - j = json.loads(open(local_copy_fn, 'r').read()) + try: + j = json.loads(open(local_copy_fn, 'r').read()) + except ValueError: + raise ValueError("Could not parse '%s'" % local_copy_fn) else: if _update_is_necessary(fn) and _update_is_possible(url): get_remote_file(fn, url) if os.path.exists(fn): - j = json.loads(open(fn, 'r').read()) + try: + j = json.loads(open(fn, 'r').read()) + except ValueError: + raise ValueError("Could not parse '%s'" % fn) return j diff -Nru click-reviewers-tools-0.10/bin/clickreviews/tests/test_cr_lint.py click-reviewers-tools-0.13/bin/clickreviews/tests/test_cr_lint.py --- click-reviewers-tools-0.10/bin/clickreviews/tests/test_cr_lint.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/tests/test_cr_lint.py 2014-10-02 15:16:19.000000000 +0000 @@ -622,6 +622,19 @@ expected_counts = {'info': 0, 'warn': 1, 'error': 0} self.check_results(r, expected_counts) + def test_check_click_local_extensions_coreapp(self): + '''Testeck_click_local_extensions() - coreapp''' + for k in self.test_manifest.keys(): + if k.startswith("x-"): + self.set_test_manifest(k, None) + self.set_test_manifest("x-source", "foo") + c = ClickReviewLint(self.test_name) + c.is_core_app = True + c.check_click_local_extensions() + r = c.click_report + expected_counts = {'info': 1, 'warn': 0, 'error': 0} + self.check_results(r, expected_counts) + def test_check_framework(self): '''Test check_framework()''' self.set_test_manifest("framework", "ubuntu-sdk-14.10-qml-dev2") @@ -697,9 +710,30 @@ c.check_hooks() r = c.click_report - expected_counts = {'info': 7, 'warn': 0, 'error': 0} + expected_counts = {'info': 13, 'warn': 0, 'error': 0} self.check_results(r, expected_counts) + def test_check_hooks_security_extension(self): + '''Test check_hooks() - security extension''' + self.set_test_manifest("framework", "ubuntu-sdk-13.10") + c = ClickReviewLint(self.test_name) + tmp = dict() + for k in c.manifest['hooks'][self.default_appname].keys(): + tmp[k] = c.manifest['hooks'][self.default_appname][k] + tmp['apparmor'] = "%s.json" % self.default_appname + c.manifest['hooks'][self.default_appname] = tmp + + c.check_hooks() + r = c.click_report + expected = dict() + expected['info'] = dict() + expected['warn'] = dict() + expected['error'] = dict() + expected['info']['lint_sdk_security_extension_test-app'] = \ + {"text": "test-app.json does not end with .apparmor (ok if not " + "using sdk)"} + self.check_results(r, expected=expected) + def test_check_hooks_bad_appname(self): '''Test check_hooks() - bad appname''' self.set_test_manifest("framework", "ubuntu-sdk-13.10") diff -Nru click-reviewers-tools-0.10/bin/clickreviews/tests/test_cr_online_accounts.py click-reviewers-tools-0.13/bin/clickreviews/tests/test_cr_online_accounts.py --- click-reviewers-tools-0.10/bin/clickreviews/tests/test_cr_online_accounts.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/tests/test_cr_online_accounts.py 2014-10-02 15:16:19.000000000 +0000 @@ -94,7 +94,32 @@ c = ClickReviewAccounts(self.test_name) c.check_application() r = c.click_report - expected_counts = {'info': 4, 'warn': 0, 'error': 0} + expected_counts = {'info': 5, 'warn': 0, 'error': 0} + self.check_results(r, expected_counts) + + def test_check_application_missing_apparmor(self): + '''Test check_application() - missing apparmor''' + xml = self._stub_application() + # print(etree.tostring(xml)) + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + del c.manifest['hooks'][self.default_appname]['apparmor'] + c.check_application() + r = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} + self.check_results(r, expected_counts) + + def test_check_application_missing_desktop_and_scope(self): + '''Test check_application() - missing desktop and scope''' + xml = self._stub_application() + # print(etree.tostring(xml)) + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + # The stub manifest doesn't have scope already + del c.manifest['hooks'][self.default_appname]['desktop'] + c.check_application() + r = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} self.check_results(r, expected_counts) def test_check_application_not_specified(self): @@ -171,14 +196,39 @@ '''Test check_service()''' xml = self._stub_service() self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report - expected_counts = {'info': 5, 'warn': 0, 'error': 0} + expected_counts = {'info': 6, 'warn': 0, 'error': 0} + self.check_results(r, expected_counts) + + def test_check_service_missing_application(self): + '''Test check_service() - missing account-application''' + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + c = ClickReviewAccounts(self.test_name) + c.check_service() + r = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} + self.check_results(r, expected_counts) + + def test_check_service_missing_apparmor(self): + '''Test check_service() - missing apparmor''' + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + del c.manifest['hooks'][self.default_appname]['apparmor'] + c.check_service() + r = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} self.check_results(r, expected_counts) def test_check_service_not_specified(self): - '''Test check_service() - no specified''' + '''Test check_service() - not specified''' c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -189,6 +239,8 @@ '''Test check_service() - wrong id''' xml = self._stub_service(id="nomatch") self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -199,6 +251,8 @@ '''Test check_service() - missing id''' xml = self._stub_service(id="") self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -209,6 +263,8 @@ '''Test check_service() - wrong root''' xml = self._stub_service(root="wrongroot") self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -223,6 +279,8 @@ service_provider = etree.SubElement(xml, "provider") service_provider.text = "some-provider" self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -237,6 +295,8 @@ service_provider = etree.SubElement(xml, "provider") service_provider.text = "some-provider" self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -251,6 +311,8 @@ service_name = etree.SubElement(xml, "name") service_name.text = "Foo" self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -261,27 +323,108 @@ '''Test check_provider()''' xml = self._stub_provider() self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) c = ClickReviewAccounts(self.test_name) c.check_provider() r = c.click_report # provider prompts manual review, so for now, need to have error as 1 - expected_counts = {'info': 2, 'warn': 0, 'error': 1} + expected_counts = {'info': 5, 'warn': 0, 'error': 1} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_missing_apparmor(self): + '''Test check_provider() - missing apparmor''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) + c = ClickReviewAccounts(self.test_name) + del c.manifest['hooks'][self.default_appname]['apparmor'] + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_missing_qml_plugin(self): + '''Test check_provider() - missing account-qml-plugin''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + c = ClickReviewAccounts(self.test_name) + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_with_application(self): + '''Test check_provider() - with account-application''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_with_service(self): + '''Test check_provider() - with account-service''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + c = ClickReviewAccounts(self.test_name) + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_with_application_and_service(self): + '''Test check_provider() - with account-application/account-service''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} self.check_results(r, expected_counts) check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) self.check_manual_review(r, check_name) def test_check_provider_not_specified(self): - '''Test check_provider() - no specified''' + '''Test check_provider() - not specified''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) c = ClickReviewAccounts(self.test_name) c.check_provider() r = c.click_report - expected_counts = {'info': 0, 'warn': 0, 'error': 0} + expected_counts = {'info': 1, 'warn': 0, 'error': 0} self.check_results(r, expected_counts) def test_check_provider_missing_id(self): '''Test check_provider() - missing id''' xml = self._stub_provider(id="") self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) c = ClickReviewAccounts(self.test_name) c.check_provider() r = c.click_report @@ -295,6 +438,7 @@ '''Test check_provider() - wrong id''' xml = self._stub_provider(id="wrongid") self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) c = ClickReviewAccounts(self.test_name) c.check_provider() r = c.click_report @@ -307,19 +451,100 @@ def test_check_qml_plugin(self): '''Test check_qml_plugin()''' self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) c = ClickReviewAccounts(self.test_name) c.check_qml_plugin() r = c.click_report # provider prompts manual review, so for now, need to have error as 1 - expected_counts = {'info': 0, 'warn': 0, 'error': 1} + expected_counts = {'info': 3, 'warn': 0, 'error': 1} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_missing_apparmor(self): + '''Test check_qml_plugin() - missing apparmor''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + c = ClickReviewAccounts(self.test_name) + del c.manifest['hooks'][self.default_appname]['apparmor'] + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_missing_provider(self): + '''Test check_qml_plugin() - missing account-provider''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + c = ClickReviewAccounts(self.test_name) + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_with_application(self): + '''Test check_qml_plugin() - with account-application''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_with_service(self): + '''Test check_qml_plugin() - with account-service''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + c = ClickReviewAccounts(self.test_name) + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_with_application_and_service(self): + '''Test check_qml_plugin() - with account-application/account-service''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 1 + expected_counts = {'info': None, 'warn': 0, 'error': 2} self.check_results(r, expected_counts) check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) self.check_manual_review(r, check_name) def test_check_qml_plugin_not_specified(self): - '''Test check_qml_plugin() - no specified''' + '''Test check_qml_plugin() - not specified''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) c = ClickReviewAccounts(self.test_name) c.check_qml_plugin() r = c.click_report - expected_counts = {'info': 0, 'warn': 0, 'error': 0} + expected_counts = {'info': 1, 'warn': 0, 'error': 0} self.check_results(r, expected_counts) diff -Nru click-reviewers-tools-0.10/bin/clickreviews/tests/test_cr_security.py click-reviewers-tools-0.13/bin/clickreviews/tests/test_cr_security.py --- click-reviewers-tools-0.10/bin/clickreviews/tests/test_cr_security.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/bin/clickreviews/tests/test_cr_security.py 2014-10-02 15:16:19.000000000 +0000 @@ -29,7 +29,7 @@ cr_tests.mock_patch() super() - self.default_security_json = "%s.json" % \ + self.default_security_json = "%s.apparmor" % \ self.default_appname def test_check_policy_version_vendor(self): @@ -286,7 +286,7 @@ report = c.click_report expected_counts = {'info': 2, 'warn': 0, 'error': 1} self.check_results(report, expected_counts) - check_name = "security_template_valid (%s.json)" % self.default_appname + check_name = "security_template_valid (%s.apparmor)" % self.default_appname self.check_manual_review(report, check_name) def test_check_policy_groups_webapps(self): @@ -416,6 +416,18 @@ expected_counts = {'info': None, 'warn': 0, 'error': 0} self.check_results(report, expected_counts) + def test_check_policy_groups_scopes_network3(self): + '''Test check_policy_groups_scopes() - network with accounts''' + self.set_test_security_manifest(self.default_appname, + "template", "ubuntu-scope-network") + self.set_test_security_manifest(self.default_appname, + "policy_groups", ["accounts"]) + c = ClickReviewSecurity(self.test_name) + c.check_policy_groups_scopes() + report = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 0} + self.check_results(report, expected_counts) + def test_check_policy_groups_scopes_network_missing(self): '''Test check_policy_groups_scopes() missing - network''' self.set_test_security_manifest(self.default_appname, @@ -433,7 +445,7 @@ self.set_test_security_manifest(self.default_appname, "template", "ubuntu-scope-network") self.set_test_security_manifest(self.default_appname, - "policy_groups", ["accounts"]) + "policy_groups", ["location"]) c = ClickReviewSecurity(self.test_name) c.check_policy_groups_scopes() report = c.click_report @@ -648,6 +660,19 @@ "push-notification-client"]) c = ClickReviewSecurity(self.test_name) c.check_policy_groups_push_helpers() + report = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} + self.check_results(report, expected_counts) + + def test_check_policy_groups_pushhelper_networking(self): + '''Test check_policy_groups_pushhelper - networking''' + self.set_test_push_helper(self.default_appname, "exec", "foo") + self.set_test_security_manifest(self.default_appname, + "policy_groups", + ["networking", + "push-notification-client"]) + c = ClickReviewSecurity(self.test_name) + c.check_policy_groups_push_helpers() report = c.click_report expected_counts = {'info': None, 'warn': 0, 'error': 1} self.check_results(report, expected_counts) diff -Nru click-reviewers-tools-0.10/clickreviews/apparmor_policy.py click-reviewers-tools-0.13/clickreviews/apparmor_policy.py --- click-reviewers-tools-0.10/clickreviews/apparmor_policy.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/apparmor_policy.py 2014-10-02 15:16:19.000000000 +0000 @@ -21,7 +21,7 @@ # XXX: This is a hack and will be gone, as soon as myapps has an API for this. AA_POLICY_DATA_URL = \ - "http://bazaar.launchpad.net/~click-reviewers/click-reviewers-tools/trunk/view/head:/data/apparmor-easyprof-ubuntu.json" + "http://bazaar.launchpad.net/~click-reviewers/click-reviewers-tools/trunk/download/head:/apparmoreasyprofubun-20140711222314-oeohtxzvf9a58fa6-1/apparmor-easyprof-ubuntu.json" def get_policy_file(fn): diff -Nru click-reviewers-tools-0.10/clickreviews/cr_common.py click-reviewers-tools-0.13/clickreviews/cr_common.py --- click-reviewers-tools-0.10/clickreviews/cr_common.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/cr_common.py 2014-10-02 15:16:19.000000000 +0000 @@ -30,15 +30,18 @@ import types DEBUGGING = False -UNPACK_DIR = None +UNPACK_DIRS = [] # cleanup import atexit def cleanup_unpack(): - if UNPACK_DIR is not None and os.path.isdir(UNPACK_DIR): - recursive_rm(UNPACK_DIR) + global UNPACK_DIRS + if len(UNPACK_DIRS) > 0: + for d in UNPACK_DIRS: + if d is not None and os.path.isdir(d): + recursive_rm(d) atexit.register(cleanup_unpack) @@ -76,8 +79,8 @@ self.click_report_output = "json" self.unpack_dir = unpack_click(fn) - global UNPACK_DIR - UNPACK_DIR = self.unpack_dir + global UNPACK_DIRS + UNPACK_DIRS.append(self.unpack_dir) # Get some basic information from the control file diff -Nru click-reviewers-tools-0.10/clickreviews/cr_desktop.py click-reviewers-tools-0.13/clickreviews/cr_desktop.py --- click-reviewers-tools-0.10/clickreviews/cr_desktop.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/cr_desktop.py 2014-10-02 15:16:19.000000000 +0000 @@ -46,6 +46,14 @@ elif 'pay-ui' in self.manifest['hooks'][app]: # msg("Skipped missing desktop hook for pay-ui '%s'" % app) continue + elif 'account-provider' in self.manifest['hooks'][app]: + # msg("Skipped missing desktop hook for account-provider" + # " '%s'" % app) + continue + elif 'account-qml-plugin' in self.manifest['hooks'][app]: + # msg("Skipped missing desktop hook for account-qml-plugin" + # " '%s'" % app) + continue else: error("could not find desktop hook for '%s'" % app) if not isinstance(self.manifest['hooks'][app]['desktop'], str): diff -Nru click-reviewers-tools-0.10/clickreviews/cr_lint.py click-reviewers-tools-0.13/clickreviews/cr_lint.py --- click-reviewers-tools-0.10/clickreviews/cr_lint.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/cr_lint.py 2014-10-02 15:16:19.000000000 +0000 @@ -342,6 +342,9 @@ mutually_exclusive = ['scope', 'desktop'] for app in self.manifest['hooks']: + t = 'info' + n = 'exclusive_hooks_%s' % (app) + s = "OK" found = [] for i in mutually_exclusive: if i in self.manifest['hooks'][app]: @@ -351,6 +354,17 @@ s = "'%s' hooks should not be used together" % ", ".join(found) self._add_result(t, n, s) + for app in self.manifest['hooks']: + if "apparmor" in self.manifest['hooks'][app]: + t = 'info' + n = 'sdk_security_extension_%s' % (app) + s = "OK" + fn = self.manifest['hooks'][app]['apparmor'] + if not fn.endswith(".apparmor"): + t = 'info' + s = '%s does not end with .apparmor (ok if not using sdk)' % fn + self._add_result(t, n, s) + def check_hooks_unknown(self): '''Check if have any unknown hooks''' t = 'info' @@ -652,14 +666,19 @@ for k in sorted(self.manifest): if k.startswith('x-'): found.append(k) + + # Some local extensions are ok for core apps, so just skip them + if self.is_core_app: + for i in ['x-source', 'x-test']: + if i in found: + found.remove(i) + if len(found) > 0: t = 'warn' plural = "" if len(found) > 1: plural = "s" s = 'found unofficial extension%s: %s' % (plural, ', '.join(found)) - if 'x-source' in k and self.is_core_app: - s += ' (x-source found, but app is a core app, which is fine)' self._add_result(t, n, s) def check_package_filename(self): diff -Nru click-reviewers-tools-0.10/clickreviews/cr_online_accounts.py click-reviewers-tools-0.13/clickreviews/cr_online_accounts.py --- click-reviewers-tools-0.10/clickreviews/cr_online_accounts.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/cr_online_accounts.py 2014-10-02 15:16:19.000000000 +0000 @@ -88,6 +88,29 @@ self._add_result(t, n, s) continue + # account-application must always appear with apparmor + t = 'info' + n = '%s_%s_apparmor' % (app, account_type) + s = "OK" + if 'apparmor' not in self.manifest['hooks'][app]: + t = 'error' + s = "missing 'apparmor' entry" + self._add_result(t, n, s) + + # account-application must always appear with desktop or scope + t = 'info' + n = '%s_%s_desktop_or_scope' % (app, account_type) + s = "OK" + found = False + for k in ['desktop', 'scope']: + if k in self.manifest['hooks'][app]: + found = True + break + if not found: + t = 'error' + s = "missing 'desktop' or 'scope' entry" + self._add_result(t, n, s) + root_tag = self.accounts[app][account_type].tag.lower() if root_tag != "application": t = 'error' @@ -141,6 +164,24 @@ self._add_result(t, n, s) continue + # account service must always appear with account-application + t = 'info' + n = '%s_%s_account-application' % (app, account_type) + s = "OK" + if 'account-application' not in self.accounts[app]: + t = 'error' + s = "missing 'account-application' entry" + self._add_result(t, n, s) + + # account-service must always appear with apparmor + t = 'info' + n = '%s_%s_apparmor' % (app, account_type) + s = "OK" + if 'apparmor' not in self.manifest['hooks'][app]: + t = 'error' + s = "missing 'apparmor' entry" + self._add_result(t, n, s) + root_tag = self.accounts[app][account_type].tag.lower() if root_tag != "service": t = 'error' @@ -192,6 +233,40 @@ manual_review = True self._add_result(t, n, s, manual_review=manual_review) + # account-provider must always appear with apparmor + t = 'info' + n = '%s_%s_apparmor' % (app, account_type) + s = "OK" + if 'apparmor' not in self.manifest['hooks'][app]: + t = 'error' + s = "missing 'apparmor' entry" + self._add_result(t, n, s) + + # account-provider must always appear with account-qml-plugin + t = 'info' + n = '%s_%s_account-qml-plugin' % (app, account_type) + s = "OK" + if 'account-qml-plugin' not in self.accounts[app]: + t = 'error' + s = "missing 'account-qml-plugin' entry" + self._add_result(t, n, s) + + # account-provider must never appear with account-application or + # account-service + t = 'info' + n = '%s_%s_account-application_or_account-service' % (app, + account_type) + s = "OK" + found = False + for i in ['account-application', 'account-service']: + if i in self.accounts[app]: + found = True + if found: + t = 'error' + s = "must not specify account-application or account-service" \ + " with %s" % account_type + self._add_result(t, n, s) + t = 'info' n = '%s_%s_root' % (app, account_type) s = "OK" @@ -233,3 +308,37 @@ s = "(MANUAL REVIEW) '%s' not allowed" % account_type manual_review = True self._add_result(t, n, s, manual_review=manual_review) + + # account-qml-plugin must always appear with apparmor + t = 'info' + n = '%s_%s_apparmor' % (app, account_type) + s = "OK" + if 'apparmor' not in self.manifest['hooks'][app]: + t = 'error' + s = "missing 'apparmor' entry" + self._add_result(t, n, s) + + # account-qml-plugin must always appear with account-provider + t = 'info' + n = '%s_%s_account-provider' % (app, account_type) + s = "OK" + if 'account-provider' not in self.accounts[app]: + t = 'error' + s = "missing 'account-provider' entry" + self._add_result(t, n, s) + + # account-qml-plugin must never appear with account-application or + # account-service + t = 'info' + n = '%s_%s_account-application_or_account-service' % (app, + account_type) + s = "OK" + found = False + for i in ['account-application', 'account-service']: + if i in self.accounts[app]: + found = True + if found: + t = 'error' + s = "must not specify account-application or account-service" \ + " with %s" % account_type + self._add_result(t, n, s) diff -Nru click-reviewers-tools-0.10/clickreviews/cr_security.py click-reviewers-tools-0.13/clickreviews/cr_security.py --- click-reviewers-tools-0.10/clickreviews/cr_security.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/cr_security.py 2014-10-02 15:16:19.000000000 +0000 @@ -16,10 +16,9 @@ from __future__ import print_function -from clickreviews.cr_common import ClickReview, error, warn +from clickreviews.cr_common import ClickReview, error import clickreviews.cr_common as cr_common import clickreviews.apparmor_policy as apparmor_policy -import glob import json import os @@ -69,6 +68,7 @@ 'webview'] self.allowed_push_helper_policy_groups = ['push-notification-client'] + self.allowed_network_scope_policy_groups = ['accounts', 'networking'] self.redflag_templates = ['unconfined'] self.extraneous_templates = ['ubuntu-sdk', @@ -396,6 +396,10 @@ for p in m['policy_groups']: if p not in self.allowed_push_helper_policy_groups: bad.append(p) + elif p == "networking": + # The above covers this, but let's be very explicit and + # never allow networking with push-notification-client + bad.append(p) if len(bad) > 0: t = 'error' s = "found unusual policy groups: %s" % ", ".join(bad) @@ -421,9 +425,8 @@ bad = [] for p in m['policy_groups']: if m['template'] == 'ubuntu-scope-network': - # networking scopes shouldn't have access to anything - # (for now, this may change with trust store (eg, location) - if p != 'networking': + # networking scopes should have extremely limited access + if p not in self.allowed_network_scope_policy_groups: bad.append(p) # jdstrand, 2014-06-05: ubuntu-scope-local-content is no longer available # elif m['template'] == 'ubuntu-scope-local-content': diff -Nru click-reviewers-tools-0.10/clickreviews/cr_tests.py click-reviewers-tools-0.13/clickreviews/cr_tests.py --- click-reviewers-tools-0.10/clickreviews/cr_tests.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/cr_tests.py 2014-10-02 15:16:19.000000000 +0000 @@ -89,7 +89,7 @@ def _get_security_manifest(self, app): '''Pretend we read the security manifest file''' - return ("%s.json" % app, json.loads(TEST_SECURITY[app])) + return ("%s.apparmor" % app, json.loads(TEST_SECURITY[app])) def _get_security_supported_policy_versions(self): @@ -289,7 +289,7 @@ self.default_appname = "test-app" self.test_manifest["hooks"][self.default_appname] = dict() self.test_manifest["hooks"][self.default_appname]["apparmor"] = \ - "%s.json" % self.default_appname + "%s.apparmor" % self.default_appname self.test_manifest["hooks"][self.default_appname]["desktop"] = \ "%s.desktop" % self.default_appname self.test_manifest["hooks"][self.default_appname]["urls"] = \ diff -Nru click-reviewers-tools-0.10/clickreviews/modules.py click-reviewers-tools-0.13/clickreviews/modules.py --- click-reviewers-tools-0.10/clickreviews/modules.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/modules.py 2014-10-02 15:16:19.000000000 +0000 @@ -68,4 +68,9 @@ init_object = find_main_class(module_name) if not init_object: return None - return init_object(click_file) + try: + ob = init_object(click_file) + except TypeError as e: + print('Could not init %s: %s' % (init_object, str(e))) + return None + return ob diff -Nru click-reviewers-tools-0.10/clickreviews/remote.py click-reviewers-tools-0.13/clickreviews/remote.py --- click-reviewers-tools-0.10/clickreviews/remote.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/remote.py 2014-10-02 15:16:19.000000000 +0000 @@ -95,10 +95,16 @@ ''' j = {} if local_copy_fn and os.path.exists(local_copy_fn): - j = json.loads(open(local_copy_fn, 'r').read()) + try: + j = json.loads(open(local_copy_fn, 'r').read()) + except ValueError: + raise ValueError("Could not parse '%s'" % local_copy_fn) else: if _update_is_necessary(fn) and _update_is_possible(url): get_remote_file(fn, url) if os.path.exists(fn): - j = json.loads(open(fn, 'r').read()) + try: + j = json.loads(open(fn, 'r').read()) + except ValueError: + raise ValueError("Could not parse '%s'" % fn) return j diff -Nru click-reviewers-tools-0.10/clickreviews/tests/test_cr_lint.py click-reviewers-tools-0.13/clickreviews/tests/test_cr_lint.py --- click-reviewers-tools-0.10/clickreviews/tests/test_cr_lint.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/tests/test_cr_lint.py 2014-10-02 15:16:19.000000000 +0000 @@ -622,6 +622,19 @@ expected_counts = {'info': 0, 'warn': 1, 'error': 0} self.check_results(r, expected_counts) + def test_check_click_local_extensions_coreapp(self): + '''Testeck_click_local_extensions() - coreapp''' + for k in self.test_manifest.keys(): + if k.startswith("x-"): + self.set_test_manifest(k, None) + self.set_test_manifest("x-source", "foo") + c = ClickReviewLint(self.test_name) + c.is_core_app = True + c.check_click_local_extensions() + r = c.click_report + expected_counts = {'info': 1, 'warn': 0, 'error': 0} + self.check_results(r, expected_counts) + def test_check_framework(self): '''Test check_framework()''' self.set_test_manifest("framework", "ubuntu-sdk-14.10-qml-dev2") @@ -697,9 +710,30 @@ c.check_hooks() r = c.click_report - expected_counts = {'info': 7, 'warn': 0, 'error': 0} + expected_counts = {'info': 13, 'warn': 0, 'error': 0} self.check_results(r, expected_counts) + def test_check_hooks_security_extension(self): + '''Test check_hooks() - security extension''' + self.set_test_manifest("framework", "ubuntu-sdk-13.10") + c = ClickReviewLint(self.test_name) + tmp = dict() + for k in c.manifest['hooks'][self.default_appname].keys(): + tmp[k] = c.manifest['hooks'][self.default_appname][k] + tmp['apparmor'] = "%s.json" % self.default_appname + c.manifest['hooks'][self.default_appname] = tmp + + c.check_hooks() + r = c.click_report + expected = dict() + expected['info'] = dict() + expected['warn'] = dict() + expected['error'] = dict() + expected['info']['lint_sdk_security_extension_test-app'] = \ + {"text": "test-app.json does not end with .apparmor (ok if not " + "using sdk)"} + self.check_results(r, expected=expected) + def test_check_hooks_bad_appname(self): '''Test check_hooks() - bad appname''' self.set_test_manifest("framework", "ubuntu-sdk-13.10") diff -Nru click-reviewers-tools-0.10/clickreviews/tests/test_cr_online_accounts.py click-reviewers-tools-0.13/clickreviews/tests/test_cr_online_accounts.py --- click-reviewers-tools-0.10/clickreviews/tests/test_cr_online_accounts.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/tests/test_cr_online_accounts.py 2014-10-02 15:16:19.000000000 +0000 @@ -94,7 +94,32 @@ c = ClickReviewAccounts(self.test_name) c.check_application() r = c.click_report - expected_counts = {'info': 4, 'warn': 0, 'error': 0} + expected_counts = {'info': 5, 'warn': 0, 'error': 0} + self.check_results(r, expected_counts) + + def test_check_application_missing_apparmor(self): + '''Test check_application() - missing apparmor''' + xml = self._stub_application() + # print(etree.tostring(xml)) + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + del c.manifest['hooks'][self.default_appname]['apparmor'] + c.check_application() + r = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} + self.check_results(r, expected_counts) + + def test_check_application_missing_desktop_and_scope(self): + '''Test check_application() - missing desktop and scope''' + xml = self._stub_application() + # print(etree.tostring(xml)) + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + # The stub manifest doesn't have scope already + del c.manifest['hooks'][self.default_appname]['desktop'] + c.check_application() + r = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} self.check_results(r, expected_counts) def test_check_application_not_specified(self): @@ -171,14 +196,39 @@ '''Test check_service()''' xml = self._stub_service() self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report - expected_counts = {'info': 5, 'warn': 0, 'error': 0} + expected_counts = {'info': 6, 'warn': 0, 'error': 0} + self.check_results(r, expected_counts) + + def test_check_service_missing_application(self): + '''Test check_service() - missing account-application''' + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + c = ClickReviewAccounts(self.test_name) + c.check_service() + r = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} + self.check_results(r, expected_counts) + + def test_check_service_missing_apparmor(self): + '''Test check_service() - missing apparmor''' + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + del c.manifest['hooks'][self.default_appname]['apparmor'] + c.check_service() + r = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} self.check_results(r, expected_counts) def test_check_service_not_specified(self): - '''Test check_service() - no specified''' + '''Test check_service() - not specified''' c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -189,6 +239,8 @@ '''Test check_service() - wrong id''' xml = self._stub_service(id="nomatch") self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -199,6 +251,8 @@ '''Test check_service() - missing id''' xml = self._stub_service(id="") self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -209,6 +263,8 @@ '''Test check_service() - wrong root''' xml = self._stub_service(root="wrongroot") self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -223,6 +279,8 @@ service_provider = etree.SubElement(xml, "provider") service_provider.text = "some-provider" self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -237,6 +295,8 @@ service_provider = etree.SubElement(xml, "provider") service_provider.text = "some-provider" self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -251,6 +311,8 @@ service_name = etree.SubElement(xml, "name") service_name.text = "Foo" self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) c = ClickReviewAccounts(self.test_name) c.check_service() r = c.click_report @@ -261,27 +323,108 @@ '''Test check_provider()''' xml = self._stub_provider() self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) c = ClickReviewAccounts(self.test_name) c.check_provider() r = c.click_report # provider prompts manual review, so for now, need to have error as 1 - expected_counts = {'info': 2, 'warn': 0, 'error': 1} + expected_counts = {'info': 5, 'warn': 0, 'error': 1} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_missing_apparmor(self): + '''Test check_provider() - missing apparmor''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) + c = ClickReviewAccounts(self.test_name) + del c.manifest['hooks'][self.default_appname]['apparmor'] + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_missing_qml_plugin(self): + '''Test check_provider() - missing account-qml-plugin''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + c = ClickReviewAccounts(self.test_name) + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_with_application(self): + '''Test check_provider() - with account-application''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_with_service(self): + '''Test check_provider() - with account-service''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + c = ClickReviewAccounts(self.test_name) + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_provider_with_application_and_service(self): + '''Test check_provider() - with account-application/account-service''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + c.check_provider() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} self.check_results(r, expected_counts) check_name = "%s_%s_account-provider" % (c.review_type, self.default_appname) self.check_manual_review(r, check_name) def test_check_provider_not_specified(self): - '''Test check_provider() - no specified''' + '''Test check_provider() - not specified''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) c = ClickReviewAccounts(self.test_name) c.check_provider() r = c.click_report - expected_counts = {'info': 0, 'warn': 0, 'error': 0} + expected_counts = {'info': 1, 'warn': 0, 'error': 0} self.check_results(r, expected_counts) def test_check_provider_missing_id(self): '''Test check_provider() - missing id''' xml = self._stub_provider(id="") self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) c = ClickReviewAccounts(self.test_name) c.check_provider() r = c.click_report @@ -295,6 +438,7 @@ '''Test check_provider() - wrong id''' xml = self._stub_provider(id="wrongid") self.set_test_account(self.default_appname, "account-provider", xml) + self.set_test_account(self.default_appname, "account-qml-plugin", True) c = ClickReviewAccounts(self.test_name) c.check_provider() r = c.click_report @@ -307,19 +451,100 @@ def test_check_qml_plugin(self): '''Test check_qml_plugin()''' self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) c = ClickReviewAccounts(self.test_name) c.check_qml_plugin() r = c.click_report # provider prompts manual review, so for now, need to have error as 1 - expected_counts = {'info': 0, 'warn': 0, 'error': 1} + expected_counts = {'info': 3, 'warn': 0, 'error': 1} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_missing_apparmor(self): + '''Test check_qml_plugin() - missing apparmor''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + c = ClickReviewAccounts(self.test_name) + del c.manifest['hooks'][self.default_appname]['apparmor'] + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_missing_provider(self): + '''Test check_qml_plugin() - missing account-provider''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + c = ClickReviewAccounts(self.test_name) + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_with_application(self): + '''Test check_qml_plugin() - with account-application''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_with_service(self): + '''Test check_qml_plugin() - with account-service''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + c = ClickReviewAccounts(self.test_name) + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 2 + expected_counts = {'info': None, 'warn': 0, 'error': 2} + self.check_results(r, expected_counts) + check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) + self.check_manual_review(r, check_name) + + def test_check_qml_plugin_with_application_and_service(self): + '''Test check_qml_plugin() - with account-application/account-service''' + self.set_test_account(self.default_appname, "account-qml-plugin", True) + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) + xml = self._stub_service() + self.set_test_account(self.default_appname, "account-service", xml) + xml = self._stub_application() + self.set_test_account(self.default_appname, "account-application", xml) + c = ClickReviewAccounts(self.test_name) + c.check_qml_plugin() + r = c.click_report + # provider prompts manual review, so for now, need to have error as 1 + expected_counts = {'info': None, 'warn': 0, 'error': 2} self.check_results(r, expected_counts) check_name = "%s_%s_account-qml-plugin" % (c.review_type, self.default_appname) self.check_manual_review(r, check_name) def test_check_qml_plugin_not_specified(self): - '''Test check_qml_plugin() - no specified''' + '''Test check_qml_plugin() - not specified''' + xml = self._stub_provider() + self.set_test_account(self.default_appname, "account-provider", xml) c = ClickReviewAccounts(self.test_name) c.check_qml_plugin() r = c.click_report - expected_counts = {'info': 0, 'warn': 0, 'error': 0} + expected_counts = {'info': 1, 'warn': 0, 'error': 0} self.check_results(r, expected_counts) diff -Nru click-reviewers-tools-0.10/clickreviews/tests/test_cr_security.py click-reviewers-tools-0.13/clickreviews/tests/test_cr_security.py --- click-reviewers-tools-0.10/clickreviews/tests/test_cr_security.py 2014-09-23 20:16:17.000000000 +0000 +++ click-reviewers-tools-0.13/clickreviews/tests/test_cr_security.py 2014-10-02 15:16:19.000000000 +0000 @@ -29,7 +29,7 @@ cr_tests.mock_patch() super() - self.default_security_json = "%s.json" % \ + self.default_security_json = "%s.apparmor" % \ self.default_appname def test_check_policy_version_vendor(self): @@ -286,7 +286,7 @@ report = c.click_report expected_counts = {'info': 2, 'warn': 0, 'error': 1} self.check_results(report, expected_counts) - check_name = "security_template_valid (%s.json)" % self.default_appname + check_name = "security_template_valid (%s.apparmor)" % self.default_appname self.check_manual_review(report, check_name) def test_check_policy_groups_webapps(self): @@ -416,6 +416,18 @@ expected_counts = {'info': None, 'warn': 0, 'error': 0} self.check_results(report, expected_counts) + def test_check_policy_groups_scopes_network3(self): + '''Test check_policy_groups_scopes() - network with accounts''' + self.set_test_security_manifest(self.default_appname, + "template", "ubuntu-scope-network") + self.set_test_security_manifest(self.default_appname, + "policy_groups", ["accounts"]) + c = ClickReviewSecurity(self.test_name) + c.check_policy_groups_scopes() + report = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 0} + self.check_results(report, expected_counts) + def test_check_policy_groups_scopes_network_missing(self): '''Test check_policy_groups_scopes() missing - network''' self.set_test_security_manifest(self.default_appname, @@ -433,7 +445,7 @@ self.set_test_security_manifest(self.default_appname, "template", "ubuntu-scope-network") self.set_test_security_manifest(self.default_appname, - "policy_groups", ["accounts"]) + "policy_groups", ["location"]) c = ClickReviewSecurity(self.test_name) c.check_policy_groups_scopes() report = c.click_report @@ -648,6 +660,19 @@ "push-notification-client"]) c = ClickReviewSecurity(self.test_name) c.check_policy_groups_push_helpers() + report = c.click_report + expected_counts = {'info': None, 'warn': 0, 'error': 1} + self.check_results(report, expected_counts) + + def test_check_policy_groups_pushhelper_networking(self): + '''Test check_policy_groups_pushhelper - networking''' + self.set_test_push_helper(self.default_appname, "exec", "foo") + self.set_test_security_manifest(self.default_appname, + "policy_groups", + ["networking", + "push-notification-client"]) + c = ClickReviewSecurity(self.test_name) + c.check_policy_groups_push_helpers() report = c.click_report expected_counts = {'info': None, 'warn': 0, 'error': 1} self.check_results(report, expected_counts) diff -Nru click-reviewers-tools-0.10/debian/bzr-builder.manifest click-reviewers-tools-0.13/debian/bzr-builder.manifest --- click-reviewers-tools-0.10/debian/bzr-builder.manifest 2014-09-23 20:16:18.000000000 +0000 +++ click-reviewers-tools-0.13/debian/bzr-builder.manifest 2014-10-02 15:16:20.000000000 +0000 @@ -1,2 +1,2 @@ -# bzr-builder format 0.3 deb-version {debupstream}-0~241 -lp:click-reviewers-tools revid:jamie@ubuntu.com-20140923173626-x6pehf6x5682wqyx +# bzr-builder format 0.3 deb-version {debupstream}-0~261 +lp:click-reviewers-tools revid:jamie@ubuntu.com-20141002132954-7mn9ouh2abil13ts diff -Nru click-reviewers-tools-0.10/debian/changelog click-reviewers-tools-0.13/debian/changelog --- click-reviewers-tools-0.10/debian/changelog 2014-09-23 20:16:18.000000000 +0000 +++ click-reviewers-tools-0.13/debian/changelog 2014-10-02 15:16:20.000000000 +0000 @@ -1,10 +1,44 @@ -click-reviewers-tools (0.10-0~241~ubuntu14.04.1) trusty; urgency=low +click-reviewers-tools (0.13-0~261~ubuntu14.04.1) trusty; urgency=low * Auto build. - -- Daniel Holbach Tue, 23 Sep 2014 20:16:18 +0000 + -- Daniel Holbach Thu, 02 Oct 2014 15:16:20 +0000 -click-reviewers-tools (0.10) UNRELEASED; urgency=medium +click-reviewers-tools (0.13) utopic; urgency=medium + + * reduce to 'info' when security policy does not end with .apparmor + (LP: #1358317) + + -- Jamie Strandboge Wed, 01 Oct 2014 08:09:42 -0500 + +click-reviewers-tools (0.12) utopic; urgency=medium + + [ Jamie Strandboge ] + * traceback in a more friendly way if the json can't be parsed + * adjust click-review --sdk to start reporting again (LP: #1375787) + * add additional tests for online accounts (LP: #1357211) + * explicitly mark 'networking' as bad policy group when using + push-notification-client (it was already implicitly bad) + + -- Jamie Strandboge Wed, 01 Oct 2014 07:14:33 -0500 + +click-reviewers-tools (0.11) utopic; urgency=medium + + [ Jamie Strandboge ] + * allow 'accounts' policy group with network scopes. + * fix fetch URL for apparmor json to point to json file, not html page + (LP: #1375326) + * check if security policy does not end with .apparmor (LP: #1358317) + * cleanup all the temp directories on shutdown (LP: #1370577) + * shouldn't warn when app is coreapp when it uses x-source or x-test + (LP: #1371180) + + [ Daniel Holbach ] + * be clearer about unloadable ClickReview classes. + + -- Jamie Strandboge Mon, 29 Sep 2014 17:01:58 -0500 + +click-reviewers-tools (0.10) utopic; urgency=medium [ Daniel Holbach ] * Split out code to find Click*Review classes in the clickreviews package @@ -20,8 +54,18 @@ hook was implemented in previous commits, but now we should adjust the cr_desktop.py hook to not error when the pay-ui hook is specified but the desktop hook is not. + * The accounts policy group is now a common policy group (14.10) and + webapps more fully integrate with accounts these days, so don't flag + accounts as unusual any more. + * Mark checks requiring manual review by using a special key in the json + data. + * Add commented out camera policy group to list of ok policygroups for + webapps. + + [ Ricardo Kirkner ] + * Updated frameworks.json using myapps api. (LP: #1363096) - -- Daniel Holbach Tue, 02 Sep 2014 18:31:50 +0200 + -- Daniel Holbach Wed, 24 Sep 2014 16:10:43 +0200 click-reviewers-tools (0.9) utopic; urgency=medium