From 7bc022ebf8f9df57e755b7f1ffb0b2be4fd6e4f1 Mon Sep 17 00:00:00 2001 From: Dheyay Date: Fri, 9 Feb 2024 11:49:37 -0800 Subject: [PATCH] Updated unit tests and behave tests --- features/unattached_status.feature | 36 ++++++----------- uaclient/cli/tests/test_cli_status.py | 4 +- uaclient/conftest.py | 12 ++++-- uaclient/files/notices.py | 55 +++++++++++++++----------- uaclient/files/tests/test_notices.py | 57 ++++++++++++++------------- uaclient/tests/test_config.py | 38 +++++++++--------- 6 files changed, 103 insertions(+), 99 deletions(-) diff --git a/features/unattached_status.feature b/features/unattached_status.feature index 1b3eef967d..f5f0154640 100644 --- a/features/unattached_status.feature +++ b/features/unattached_status.feature @@ -602,35 +602,23 @@ Feature: Unattached status Scenario Outline: Check notice file read permission Given a `` `` machine with ubuntu-advantage-tools installed - When I run `rm -rf /run/ubuntu-advantage` with sudo When I run `mkdir -p /run/ubuntu-advantage/notices` with sudo When I run `touch /run/ubuntu-advantage/notices/crasher` with sudo When I run `chmod 0 /run/ubuntu-advantage/notices/crasher` with sudo - When I run `rm -rf /var/ubuntu-advantage` with sudo - When I run `mkdir -p /var/ubuntu-advantage/notices` with sudo - When I run `touch /var/ubuntu-advantage/notices/crasher` with sudo - When I run `chmod 0 /var/ubuntu-advantage/notices/crasher` with sudo + When I run `mkdir -p /var/lib/ubuntu-advantage/notices` with sudo + When I run `touch /var/lib/ubuntu-advantage/notices/crasher` with sudo + When I run `chmod 0 /var/lib/ubuntu-advantage/notices/crasher` with sudo + When I run `touch /var/lib/ubuntu-advantage/notices/10-reboot_required` with sudo When I run `pro status` as non-root Then stdout matches regexp: """ - SERVICE +AVAILABLE +DESCRIPTION - (anbox-cloud +(yes|no) +.*)? - ?cc-eal +yes +Common Criteria EAL2 Provisioning Packages - cis +yes +Security compliance and audit tools - esm-apps +yes +Expanded Security Maintenance for Applications - esm-infra +yes +Expanded Security Maintenance for Infrastructure - fips +yes +NIST-certified FIPS crypto packages - fips-updates +yes +FIPS compliant crypto packages with stable security updates - livepatch +yes +(Canonical Livepatch service|Current kernel is not supported) - ros +yes +Security Updates for the Robot Operating System - ros-updates +yes +All Updates for the Robot Operating System - - For a list of all Ubuntu Pro services, run 'pro status --all' - - This machine is not attached to an Ubuntu Pro subscription. - See https://ubuntu.com/pro + NOTICES + System reboot required """ Examples: ubuntu release - | release | machine_type | - | xenial | lxd-container | - | bionic | lxd-container | \ No newline at end of file + | release | machine_type | + | xenial | lxd-container | + | bionic | lxd-container | + | focal | lxd-container | + | jammy | lxd-container | + | mantic | lxd-container | diff --git a/uaclient/cli/tests/test_cli_status.py b/uaclient/cli/tests/test_cli_status.py index 4085cb8b88..9cfb33a744 100644 --- a/uaclient/cli/tests/test_cli_status.py +++ b/uaclient/cli/tests/test_cli_status.py @@ -372,8 +372,8 @@ def test_status_help( ([], ""), ( [ - [FakeNotice.a, "adesc"], - [FakeNotice.b, "bdesc"], + [FakeNotice.reboot_required, "adesc"], + [FakeNotice.enable_reboot_required, "bdesc"], ], "\nNOTICES\nadesc\nbdesc\n", ), diff --git a/uaclient/conftest.py b/uaclient/conftest.py index b9b5d4e605..6c6a3e09d9 100644 --- a/uaclient/conftest.py +++ b/uaclient/conftest.py @@ -285,9 +285,15 @@ def event(): class FakeNotice(NoticeFileDetails, Enum): - a = NoticeFileDetails("01", "a", True, "notice_a") - a2 = NoticeFileDetails("03", "a2", True, "notice_a2") - b = NoticeFileDetails("02", "b2", False, "notice_b") + reboot_required = NoticeFileDetails( + "10", "reboot_required", False, "notice_a" + ) + reboot_script_failed = NoticeFileDetails( + "12", "reboot_script_failed", True, "notice_a2" + ) + enable_reboot_required = NoticeFileDetails( + "11", "enable_reboot_required", False, "notice_b" + ) @pytest.yield_fixture(autouse=True) diff --git a/uaclient/files/notices.py b/uaclient/files/notices.py index 1485d58aca..da90d0de70 100644 --- a/uaclient/files/notices.py +++ b/uaclient/files/notices.py @@ -170,16 +170,36 @@ def _get_notice_file_names(self, directory: str) -> List[str]: file_name for file_name in os.listdir(directory) if os.path.isfile(os.path.join(directory, file_name)) + and self._is_valid_notice_file(directory, file_name) ] - def _is_readable(self, directory: str, file_name: str) -> bool: - """Checks if the notice file is readable. + def _is_valid_notice_file(self, directory: str, file_name: str) -> bool: + """Checks if the notice file is valid. - :param directory: The directory where the notice file is located. :param file_name: The name of the notice file. - :returns: True if the file is readable, False otherwise. + :returns: True if the file is valid, False otherwise. """ - return os.access(os.path.join(directory, file_name), os.R_OK) + try: + order_id, label = file_name.split("-") + for n in Notice: + if ( + n.order_id == order_id + and n.label == label + and ( + ( + directory == defaults.NOTICES_PERMANENT_DIRECTORY + and n.is_permanent + ) + or ( + directory == defaults.NOTICES_TEMPORARY_DIRECTORY + and not n.is_permanent + ) + ) + ): + return True + return False + except ValueError: + return False def _get_default_message(self, file_name: str) -> str: """Gets the default message for a notice file. @@ -187,21 +207,12 @@ def _get_default_message(self, file_name: str) -> str: :param file_name: The name of the notice file. :returns: The default message defined in the enum. """ - try: - order_id, label = file_name.split("-") - notice = None - for n in Notice: - if n.order_id == order_id and n.label == label: - notice = n - if notice is None: - raise Exception() - return notice.value.message - except Exception: - LOG.warning( - "Something went wrong while processing notice: %s.", - file_name, - ) - return "" + order_id, label = file_name.split("-") + notice = None + for notice in Notice: + if notice.order_id == order_id and notice.label == label: + return notice.value.message + return "" def list(self) -> List[str]: """Gets all the notice files currently saved. @@ -218,15 +229,13 @@ def list(self) -> List[str]: continue notice_file_names = self._get_notice_file_names(notice_directory) for notice_file_name in notice_file_names: - if not self._is_readable(notice_directory, notice_file_name): - continue try: notice_file_contents = system.load_file( os.path.join(notice_directory, notice_file_name) ) except PermissionError: LOG.warning( - f"Permission error while reading {notice_file_name}" + "Permission error while reading " + notice_file_name ) continue if notice_file_contents: diff --git a/uaclient/files/tests/test_notices.py b/uaclient/files/tests/test_notices.py index 0edea463da..d81edcfc7e 100644 --- a/uaclient/files/tests/test_notices.py +++ b/uaclient/files/tests/test_notices.py @@ -14,8 +14,8 @@ class TestNotices: "label,content", ( ( - FakeNotice.a, - "notice_a", + FakeNotice.reboot_script_failed, + "notice_a2", ), ), ) @@ -30,7 +30,10 @@ def test_add( notice.add(label, content) assert [ mock.call( - os.path.join(defaults.NOTICES_PERMANENT_DIRECTORY, "01-a"), + os.path.join( + defaults.NOTICES_PERMANENT_DIRECTORY, + "12-reboot_script_failed", + ), content, ) ] == sys_write_file.call_args_list @@ -44,15 +47,18 @@ def test_add_non_root( caplog_text, ): notice = NoticesManager() - notice.add(FakeNotice.a, "content") + notice.add(FakeNotice.reboot_required, "content") assert [] == m_sys_write_file.call_args_list - assert "NoticesManager.add(a) called as non-root user" in caplog_text() + assert ( + "NoticesManager.add(reboot_required) called as non-root user" + in caplog_text() + ) @pytest.mark.parametrize( "label,content", ( ( - FakeNotice.a, + FakeNotice.reboot_required, "notice_a", ), ), @@ -74,8 +80,8 @@ def test_add_duplicate_label( "label,content", ( ( - FakeNotice.a, - "notice_a", + FakeNotice.reboot_script_failed, + "notice_a2", ), ), ) @@ -91,7 +97,10 @@ def test_remove( notice.remove(label) assert [ mock.call( - os.path.join(defaults.NOTICES_PERMANENT_DIRECTORY, "01-a"), + os.path.join( + defaults.NOTICES_PERMANENT_DIRECTORY, + "12-reboot_script_failed", + ), ) ] == sys_file_absent.call_args_list @@ -104,10 +113,11 @@ def test_remove_non_root( caplog_text, ): notice = NoticesManager() - notice.remove(FakeNotice.a) + notice.remove(FakeNotice.reboot_required) assert [] == m_sys_file_absent.call_args_list assert ( - "NoticesManager.remove(a) called as non-root user" in caplog_text() + "NoticesManager.remove(reboot_required) called as non-root user" + in caplog_text() ) @mock.patch("uaclient.files.notices.NoticesManager.list") @@ -116,12 +126,14 @@ def test_remove_non_root( def test_notice_module( self, notice_cls_add, notice_cls_remove, notice_cls_read ): - notices.add(FakeNotice.a) + notices.add(FakeNotice.reboot_required) assert [ - mock.call(FakeNotice.a, "notice_a"), + mock.call(FakeNotice.reboot_required, "notice_a"), ] == notice_cls_add.call_args_list - notices.remove(FakeNotice.a) - assert [mock.call(FakeNotice.a)] == notice_cls_remove.call_args_list + notices.remove(FakeNotice.reboot_required) + assert [ + mock.call(FakeNotice.reboot_required) + ] == notice_cls_remove.call_args_list notices.list() assert 1 == notice_cls_read.call_count @@ -135,27 +147,16 @@ def test_get_notice_file_names(self, m_get_notice_file_names): "directory" ) - @mock.patch("uaclient.files.notices.os.access") - def test_is_readable(self, m_access): - notice = NoticesManager() - m_access.return_value = False - assert not notice._is_readable("directory", "file") - assert [] == notice.list() - m_access.assert_called_once_with("directory/file", os.R_OK) - @mock.patch("uaclient.files.notices.NoticesManager._get_notice_file_names") - @mock.patch("uaclient.files.notices.NoticesManager._is_readable") @mock.patch("uaclient.system.load_file") - def test_list(self, m_load_file, m_is_readable, m_get_notice_file_names): + def test_list(self, m_load_file, m_get_notice_file_names): notice = NoticesManager() m_get_notice_file_names.side_effect = ( lambda directory: [] if directory == defaults.NOTICES_TEMPORARY_DIRECTORY - else ["fakeNotice1", "fakeNotice2"] + else ["fakeNotice1"] ) - m_is_readable.side_effect = [True, False] m_load_file.return_value = "test" assert ["test"] == notice.list() - assert 2 == notice._is_readable.call_count diff --git a/uaclient/tests/test_config.py b/uaclient/tests/test_config.py index 6104c9e439..89ea27f042 100644 --- a/uaclient/tests/test_config.py +++ b/uaclient/tests/test_config.py @@ -74,13 +74,13 @@ class TestNotices: ( ([], ()), ( - [[FakeNotice.a2, "a1"]], + [[FakeNotice.reboot_script_failed, "a1"]], ["a1"], ), ( [ - [FakeNotice.a, "a1"], - [FakeNotice.a2, "a2"], + [FakeNotice.reboot_required, "a1"], + [FakeNotice.reboot_script_failed, "a2"], ], [ "a1", @@ -89,8 +89,8 @@ class TestNotices: ), ( [ - [FakeNotice.a, "a1"], - [FakeNotice.a, "a1"], + [FakeNotice.reboot_required, "a1"], + [FakeNotice.reboot_required, "a1"], ], [ "a1", @@ -116,11 +116,11 @@ def test_add_notice_avoids_duplicates( "_notices", ( ([]), - ([[FakeNotice.a]]), + ([[FakeNotice.reboot_required]]), ( [ - [FakeNotice.a], - [FakeNotice.a2], + [FakeNotice.reboot_required], + [FakeNotice.reboot_script_failed], ] ), ), @@ -139,29 +139,29 @@ def test_add_notice_fails_as_nonroot( @pytest.mark.parametrize( "notices_,removes,expected", ( - ([], [FakeNotice.a], []), + ([], [FakeNotice.reboot_required], []), ( - [[FakeNotice.a2]], - [FakeNotice.a2], + [[FakeNotice.reboot_script_failed]], + [FakeNotice.reboot_script_failed], [], ), ( [ - [FakeNotice.a], - [FakeNotice.a2], + [FakeNotice.reboot_required], + [FakeNotice.reboot_script_failed], ], - [FakeNotice.a], + [FakeNotice.reboot_required], ["notice_a2"], ), ( [ - [FakeNotice.a], - [FakeNotice.a2], - [FakeNotice.b], + [FakeNotice.reboot_required], + [FakeNotice.reboot_script_failed], + [FakeNotice.enable_reboot_required], ], [ - FakeNotice.a, - FakeNotice.a2, + FakeNotice.reboot_required, + FakeNotice.reboot_script_failed, ], ["notice_b"], ),