From f55b1434c239980eeea37f9fa37238e0715340c0 Mon Sep 17 00:00:00 2001 From: shatakshiiii Date: Mon, 28 Aug 2023 21:01:05 +0530 Subject: [PATCH 1/7] Transform for partial-become rule --- .github/workflows/tox.yml | 2 +- .../transform-partial-become.transformed.yml | 31 +++++ .../playbooks/transform-partial-become.yml | 28 ++++ src/ansiblelint/rules/partial_become.md | 11 +- src/ansiblelint/rules/partial_become.py | 122 +++++++----------- src/ansiblelint/schemas/__store__.json | 2 +- test/test_transformer.py | 6 + 7 files changed, 124 insertions(+), 78 deletions(-) create mode 100644 examples/playbooks/transform-partial-become.transformed.yml create mode 100644 examples/playbooks/transform-partial-become.yml diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index 204145a44d..902199c5f0 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -71,7 +71,7 @@ jobs: WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 813 + PYTEST_REQPASS: 815 steps: - name: Activate WSL1 if: "contains(matrix.shell, 'wsl')" diff --git a/examples/playbooks/transform-partial-become.transformed.yml b/examples/playbooks/transform-partial-become.transformed.yml new file mode 100644 index 0000000000..ddcd463b9b --- /dev/null +++ b/examples/playbooks/transform-partial-become.transformed.yml @@ -0,0 +1,31 @@ +--- +- hosts: localhost + name: Use of become_user without become play + become: true + become_user: root + + tasks: + - ansible.builtin.debug: + msg: hello + +- hosts: localhost + + tasks: + - name: Use of become_user without become task + ansible.builtin.command: whoami + become: true + become_user: postgres + changed_when: false + +- hosts: localhost + + tasks: + - name: A block with become and become_user on different tasks + block: + - name: Sample become + become: true + ansible.builtin.command: whoami + - name: Sample become_user + become: true + become_user: postgres + ansible.builtin.command: whoami diff --git a/examples/playbooks/transform-partial-become.yml b/examples/playbooks/transform-partial-become.yml new file mode 100644 index 0000000000..da48b2f967 --- /dev/null +++ b/examples/playbooks/transform-partial-become.yml @@ -0,0 +1,28 @@ +--- +- hosts: localhost + name: Use of become_user without become play + become_user: root + + tasks: + - ansible.builtin.debug: + msg: hello + +- hosts: localhost + + tasks: + - name: Use of become_user without become task + ansible.builtin.command: whoami + become_user: postgres + changed_when: false + +- hosts: localhost + + tasks: + - name: A block with become and become_user on different tasks + block: + - name: Sample become + become: true + ansible.builtin.command: whoami + - name: Sample become_user + become_user: postgres + ansible.builtin.command: whoami diff --git a/src/ansiblelint/rules/partial_become.md b/src/ansiblelint/rules/partial_become.md index 01f9dae546..9f1c0a27ae 100644 --- a/src/ansiblelint/rules/partial_become.md +++ b/src/ansiblelint/rules/partial_become.md @@ -5,6 +5,11 @@ This rule checks that privilege escalation is activated when changing users. To perform an action as a different user with the `become_user` directive, you must set `become: true`. +This rule can produce the following messages: + +- `partial-become[play]`: become_user requires become to work as expected, at play level. +- `partial-become[task]`: become_user requires become to work as expected, at task level. + !!! warning While Ansible inherits have of `become` and `become_user` from upper levels, @@ -24,7 +29,7 @@ must set `become: true`. ansible.builtin.service: name: httpd state: started - become_user: apache # <- Does not change the user because "become: true" is not set. + become_user: apache # <- Does not change the user because "become: true" is not set. ``` ## Correct Code @@ -37,6 +42,6 @@ must set `become: true`. ansible.builtin.service: name: httpd state: started - become: true # <- Activates privilege escalation. - become_user: apache # <- Changes the user with the desired privileges. + become: true # <- Activates privilege escalation. + become_user: apache # <- Changes the user with the desired privileges. ``` diff --git a/src/ansiblelint/rules/partial_become.py b/src/ansiblelint/rules/partial_become.py index d14c06f31b..85101b511b 100644 --- a/src/ansiblelint/rules/partial_become.py +++ b/src/ansiblelint/rules/partial_become.py @@ -21,78 +21,20 @@ from __future__ import annotations import sys -from functools import reduce from typing import TYPE_CHECKING, Any from ansiblelint.constants import LINE_NUMBER_KEY -from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task -def _get_subtasks(data: dict[str, Any]) -> list[Any]: - result: list[Any] = [] - block_names = [ - "tasks", - "pre_tasks", - "post_tasks", - "handlers", - "block", - "always", - "rescue", - ] - for name in block_names: - if data and name in data: - result += data[name] or [] - return result - - -def _nested_search(term: str, data: dict[str, Any]) -> Any: - if data and term in data: - return True - return reduce( - (lambda x, y: x or _nested_search(term, y)), - _get_subtasks(data), - False, - ) - - -def _become_user_without_become(becomeuserabove: bool, data: dict[str, Any]) -> Any: - if "become" in data: - # If become is in lineage of tree then correct - return False - if "become_user" in data and _nested_search("become", data): - # If 'become_user' on tree and become somewhere below - # we must check for a case of a second 'become_user' without a - # 'become' in its lineage - subtasks = _get_subtasks(data) - return reduce( - (lambda x, y: x or _become_user_without_become(False, y)), - subtasks, - False, - ) - if _nested_search("become_user", data): - # Keep searching down if 'become_user' exists in the tree below current task - subtasks = _get_subtasks(data) - return len(subtasks) == 0 or reduce( - ( - lambda x, y: x - or _become_user_without_become( - becomeuserabove or "become_user" in data, - y, - ) - ), - subtasks, - False, - ) - # If at bottom of tree, flag up if 'become_user' existed in the lineage of the tree and - # 'become' was not. This is an error if any lineage has a 'become_user' but no become - return becomeuserabove - - -class BecomeUserWithoutBecomeRule(AnsibleLintRule): +class BecomeUserWithoutBecomeRule(AnsibleLintRule, TransformMixin): """become_user requires become to work as expected.""" id = "partial-become" @@ -102,18 +44,52 @@ class BecomeUserWithoutBecomeRule(AnsibleLintRule): version_added = "historic" def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: - if file.kind == "playbook": - result = _become_user_without_become(False, data) - if result: - return [ - self.create_matcherror( - message=self.shortdesc, - filename=file, - lineno=data[LINE_NUMBER_KEY], - ), - ] + if ( + file.kind == "playbook" + and data.get("become_user") + and not data.get("become") + ): + return [ + self.create_matcherror( + message=self.shortdesc, + filename=file, + tag=f"{self.id}[play]", + lineno=data[LINE_NUMBER_KEY], + ), + ] return [] + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + if task.get("become_user") and not task.get("become"): + return [ + self.create_matcherror( + message=self.shortdesc, + filename=file, + tag=f"{self.id}[task]", + lineno=task[LINE_NUMBER_KEY], + ), + ] + return [] + + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if match.tag in ("partial-become[play]", "partial-become[task]"): + target_task = self.seek(match.yaml_path, data) + for _ in range(len(target_task)): + k, v = target_task.popitem(False) + if k == "become_user": + target_task["become"] = True + target_task[k] = v + match.fixed = True + # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: diff --git a/src/ansiblelint/schemas/__store__.json b/src/ansiblelint/schemas/__store__.json index ad9e85351b..889acb8ed0 100644 --- a/src/ansiblelint/schemas/__store__.json +++ b/src/ansiblelint/schemas/__store__.json @@ -48,7 +48,7 @@ "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/role-arg-spec.json" }, "rulebook": { - "etag": "607eeed6d517b942b4cf86c9b2f811757feccd9f7163e5298ebc8e1f2537c748", + "etag": "ac546675ec70a8c0742b3502c9dc296a508c3dda9df09ca392b8e472fa8f9823", "url": "https://raw.githubusercontent.com/ansible/ansible-rulebook/main/ansible_rulebook/schema/ruleset_schema.json" }, "tasks": { diff --git a/test/test_transformer.py b/test/test_transformer.py index 61e8112948..874c8eaf64 100644 --- a/test/test_transformer.py +++ b/test/test_transformer.py @@ -96,6 +96,12 @@ def fixture_runner_result( True, id="cmd_instead_of_shell", ), + pytest.param( + "examples/playbooks/transform-partial-become.yml", + 8, + True, + id="partial_become", + ), ), ) def test_transformer( # pylint: disable=too-many-arguments, too-many-locals From 799cca0dd35577bb42a66b361f1234e0304394bc Mon Sep 17 00:00:00 2001 From: shatakshiiii Date: Wed, 30 Aug 2023 15:05:44 +0530 Subject: [PATCH 2/7] Fixes for related tests --- ...rule-partial-become-without-become-pass.yml | 2 ++ src/ansiblelint/schemas/__store__.json | 18 +++++++++--------- test/test_skip_inside_yaml.py | 2 +- test/test_yaml_utils.py | 8 ++++---- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/examples/playbooks/rule-partial-become-without-become-pass.yml b/examples/playbooks/rule-partial-become-without-become-pass.yml index e1ae189f63..b580b43c5f 100644 --- a/examples/playbooks/rule-partial-become-without-become-pass.yml +++ b/examples/playbooks/rule-partial-become-without-become-pass.yml @@ -23,10 +23,12 @@ - name: Accepts a become from higher scope ansible.builtin.command: whoami become_user: postgres + become: true changed_when: false - hosts: localhost become_user: postgres + become: true tasks: - name: Accepts a become from a lower scope diff --git a/src/ansiblelint/schemas/__store__.json b/src/ansiblelint/schemas/__store__.json index c96ff7dadc..9f1d58a7f3 100644 --- a/src/ansiblelint/schemas/__store__.json +++ b/src/ansiblelint/schemas/__store__.json @@ -1,6 +1,6 @@ { "ansible-lint-config": { - "etag": "786b5e9ad1a19bebdb964840a32b921201a80adcab0a04fcbd4c926fc9e90335", + "etag": "616457d973f4b221d85910086ce1fab520d8a363b1e1e62a62e4d8b57a824976", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/ansible-lint-config.json" }, "ansible-navigator-config": { @@ -12,19 +12,19 @@ "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/changelog.json" }, "execution-environment": { - "etag": "f3abb1716134227ccd667607840dd7bdebfd02a8980603df031282126dc78264", + "etag": "2e1b1d02460fb93892252439e9634d9574dfdd37aea82af32f4622dacd5990b5", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/execution-environment.json" }, "galaxy": { - "etag": "61f38feb51dc7eaff43ab22f3759b3a5202776ee75ee4204f07135282817f724", + "etag": "a842f41dee838f4cf19d24d5b056a785fd2ea3449bbd899f82b6ba41ba1068db", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/galaxy.json" }, "inventory": { - "etag": "3dcd4890bf31e634a7c4f6138286a42b4985393f210f7ffaa840c2127876aa55", + "etag": "b52c251a121e2e807928db7b4e09338babde9e74a50d0f74e8908f6e230d101d", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/inventory.json" }, "meta": { - "etag": "0f376059285181985711b4271a6ff34a8dde662b9fc221d09bdcd64e4fbf86bf", + "etag": "d1f66f20b45ecb814d9be900f01451d8035059b312de4a19edc64e29811a7f39", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/meta.json" }, "meta-runtime": { @@ -32,7 +32,7 @@ "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/meta-runtime.json" }, "molecule": { - "etag": "3456b2e5aaa02fde359ff147cff81d01a37c07f5e10542b6b8b61aaaf8c756a6", + "etag": "3b625438c28e884ac42a14c09ca542fc3e1b4466abaf47d0c28646e0857d3fb5", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/molecule.json" }, "playbook": { @@ -40,11 +40,11 @@ "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/playbook.json" }, "requirements": { - "etag": "93c6ccd1f79f58134795b85f9b1193d6e18417dd01a9d1f37d9f247562a1e6fe", + "etag": "5ae3a6058ac626a341338c760db7cef7f02a8911c7293c7e129dbc6b0f8bb86d", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/requirements.json" }, "role-arg-spec": { - "etag": "498a6f716c7e99bd474ae9e7d34b3f43fbf2aad750f769392fc8e29fa590be6c", + "etag": "5113ade1d42ab250729da4a24772dd8abe8777d91f241388e4bf6033fe91ef98", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/role-arg-spec.json" }, "rulebook": { @@ -56,7 +56,7 @@ "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/tasks.json" }, "vars": { - "etag": "5d6c2c22a58f2b48c2a8d8d129f2516e4f17ffc78a2c9ba045eb5ede0ff749d7", + "etag": "73feaa77561d1d5b0bebe6cd66d499a28d67037055ac6d746139a38c9d28ca04", "url": "https://raw.githubusercontent.com/ansible/ansible-lint/main/src/ansiblelint/schemas/vars.json" } } diff --git a/test/test_skip_inside_yaml.py b/test/test_skip_inside_yaml.py index 363734e089..f8b889c270 100644 --- a/test/test_skip_inside_yaml.py +++ b/test/test_skip_inside_yaml.py @@ -19,7 +19,7 @@ def test_role_tasks_with_block(default_rules_collection: RulesCollection) -> Non @pytest.mark.parametrize( ("lintable", "expected"), - (pytest.param("examples/playbooks/test_skip_inside_yaml.yml", 4, id="yaml"),), + (pytest.param("examples/playbooks/test_skip_inside_yaml.yml", 6, id="yaml"),), ) def test_inline_skips( default_rules_collection: RulesCollection, diff --git a/test/test_yaml_utils.py b/test/test_yaml_utils.py index 6a6c7e7635..ae3a2b68a1 100644 --- a/test/test_yaml_utils.py +++ b/test/test_yaml_utils.py @@ -422,7 +422,7 @@ def fixture_ruamel_data(lintable: Lintable) -> CommentedMap | CommentedSeq: ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 28, + 29, [3], id="4_play_playbook-first_line_in_play_4", ), @@ -675,7 +675,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 28, + 29, [], id="4_play_playbook-play_4_line_before_tasks", ), @@ -687,13 +687,13 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 32, + 34, [3, "tasks", 0], id="4_play_playbook-play_4_first_line_task_1", ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 33, + 35, [3, "tasks", 0], id="4_play_playbook-play_4_middle_line_task_1", ), From 19294ef847d71523de26f9b673b766e55b938144 Mon Sep 17 00:00:00 2001 From: "Bradley A. Thornton" Date: Thu, 31 Aug 2023 15:28:38 -0700 Subject: [PATCH 3/7] Updates --- ...ule-partial-become-without-become-fail.yml | 21 +- ...ule-partial-become-without-become-pass.yml | 20 +- .../tasks/patial_become.yml/main.yml | 3 + .../transform-partial-become.transformed.yml | 69 +++++-- .../playbooks/transform-partial-become.yml | 64 ++++-- src/ansiblelint/rules/partial_become.md | 79 ++++++- src/ansiblelint/rules/partial_become.py | 195 +++++++++++++++--- test/test_transformer.py | 2 +- 8 files changed, 356 insertions(+), 97 deletions(-) create mode 100644 examples/playbooks/tasks/patial_become.yml/main.yml diff --git a/examples/playbooks/rule-partial-become-without-become-fail.yml b/examples/playbooks/rule-partial-become-without-become-fail.yml index da48b2f967..80b633d908 100644 --- a/examples/playbooks/rule-partial-become-without-become-fail.yml +++ b/examples/playbooks/rule-partial-become-without-become-fail.yml @@ -1,28 +1,27 @@ --- -- hosts: localhost - name: Use of become_user without become play +- name: Use of become_user without become at play level + hosts: localhost become_user: root tasks: - - ansible.builtin.debug: + - name: A task without issues + ansible.builtin.debug: msg: hello -- hosts: localhost - +- name: Use of become_user without become at task level + hosts: localhost tasks: - name: Use of become_user without become task ansible.builtin.command: whoami become_user: postgres changed_when: false -- hosts: localhost - +- name: Use of become_user without become at task level + hosts: localhost tasks: - name: A block with become and become_user on different tasks block: - name: Sample become - become: true - ansible.builtin.command: whoami - - name: Sample become_user - become_user: postgres ansible.builtin.command: whoami + become_user: true + changed_when: false diff --git a/examples/playbooks/rule-partial-become-without-become-pass.yml b/examples/playbooks/rule-partial-become-without-become-pass.yml index b580b43c5f..11a0d2be3d 100644 --- a/examples/playbooks/rule-partial-become-without-become-pass.yml +++ b/examples/playbooks/rule-partial-become-without-become-pass.yml @@ -1,14 +1,16 @@ --- -- hosts: localhost +- name: Test play + hosts: localhost become_user: root become: true tasks: - - ansible.builtin.debug: + - name: debug + ansible.builtin.debug: msg: hello -- hosts: localhost - +- name: Test play + hosts: localhost tasks: - name: Foo ansible.builtin.command: whoami @@ -16,17 +18,16 @@ become: true changed_when: false -- hosts: localhost - become: true +- name: Test play + hosts: localhost tasks: - name: Accepts a become from higher scope ansible.builtin.command: whoami - become_user: postgres - become: true changed_when: false -- hosts: localhost +- name: Test play + hosts: localhost become_user: postgres become: true @@ -34,4 +35,5 @@ - name: Accepts a become from a lower scope ansible.builtin.command: whoami become: true + become_user: root changed_when: false diff --git a/examples/playbooks/tasks/patial_become.yml/main.yml b/examples/playbooks/tasks/patial_become.yml/main.yml new file mode 100644 index 0000000000..622ed88338 --- /dev/null +++ b/examples/playbooks/tasks/patial_become.yml/main.yml @@ -0,0 +1,3 @@ +- name: Included with partial become + ansible.builtin.debug: + msg: Included with partial become diff --git a/examples/playbooks/transform-partial-become.transformed.yml b/examples/playbooks/transform-partial-become.transformed.yml index ddcd463b9b..066c39ad7b 100644 --- a/examples/playbooks/transform-partial-become.transformed.yml +++ b/examples/playbooks/transform-partial-become.transformed.yml @@ -1,31 +1,56 @@ --- -- hosts: localhost - name: Use of become_user without become play - become: true - become_user: root - +# The play has become_user and the task has become +# this is fixable, copy the become_user to the task +# and remove from the play +- name: Play 1 + hosts: localhost tasks: - - ansible.builtin.debug: - msg: hello - -- hosts: localhost + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become: true + become_user: root +# The task has become_user but the play does not +# this is fixable, remove the become_user from the task +- name: Play 2 + hosts: localhost tasks: - - name: Use of become_user without become task - ansible.builtin.command: whoami - become: true - become_user: postgres - changed_when: false - -- hosts: localhost + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello +# The task has become_user and the play has become +# this is fixable, add become to the task +- name: Play 3 + hosts: localhost + become: true tasks: - - name: A block with become and become_user on different tasks + - name: A block block: - - name: Sample become + - name: Debug + ansible.builtin.debug: + msg: hello become: true - ansible.builtin.command: whoami - - name: Sample become_user + become_user: root + +# The play has become_user but has an include +# this is unfixable, the include could be called from multiple playbooks +- name: Play 4 + hosts: localhost + become_user: root + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello become: true - become_user: postgres - ansible.builtin.command: whoami + + - name: Include + ansible.builtin.include_tasks: + file: ../tasks/partial_become/main.yml diff --git a/examples/playbooks/transform-partial-become.yml b/examples/playbooks/transform-partial-become.yml index da48b2f967..b1e4f39d13 100644 --- a/examples/playbooks/transform-partial-become.yml +++ b/examples/playbooks/transform-partial-become.yml @@ -1,28 +1,56 @@ --- -- hosts: localhost - name: Use of become_user without become play +# The play has become_user and the task has become +# this is fixable, copy the become_user to the task +# and remove from the play +- name: Play 1 + hosts: localhost become_user: root - tasks: - - ansible.builtin.debug: - msg: hello - -- hosts: localhost + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become: true +# The task has become_user but the play does not +# this is fixable, remove the become_user from the task +- name: Play 2 + hosts: localhost tasks: - - name: Use of become_user without become task - ansible.builtin.command: whoami - become_user: postgres - changed_when: false + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become_user: root -- hosts: localhost +# The task has become_user and the play has become +# this is fixable, add become to the task +- name: Play 3 + hosts: localhost + become: true + tasks: + - name: A block + block: + - name: Debug + ansible.builtin.debug: + msg: hello + become_user: root +# The play has become_user but has an include +# this is unfixable, the include could be called from multiple playbooks +- name: Play 4 + hosts: localhost + become_user: root tasks: - - name: A block with become and become_user on different tasks + - name: A block block: - - name: Sample become + - name: Debug + ansible.builtin.debug: + msg: hello become: true - ansible.builtin.command: whoami - - name: Sample become_user - become_user: postgres - ansible.builtin.command: whoami + + - name: Include + ansible.builtin.include_tasks: + file: ../tasks/partial_become/main.yml diff --git a/src/ansiblelint/rules/partial_become.md b/src/ansiblelint/rules/partial_become.md index 9f1c0a27ae..7ff20e583e 100644 --- a/src/ansiblelint/rules/partial_become.md +++ b/src/ansiblelint/rules/partial_become.md @@ -7,8 +7,10 @@ must set `become: true`. This rule can produce the following messages: -- `partial-become[play]`: become_user requires become to work as expected, at play level. -- `partial-become[task]`: become_user requires become to work as expected, at task level. +- `partial-become[play]`: become_user requires become to work as expected, at + play level. +- `partial-become[task]`: become_user requires become to work as expected, at + task level. !!! warning @@ -24,6 +26,7 @@ This rule can produce the following messages: --- - name: Example playbook hosts: localhost + become: true # <- Activates privilege escalation. tasks: - name: Start the httpd service as the apache user ansible.builtin.service: @@ -44,4 +47,76 @@ This rule can produce the following messages: state: started become: true # <- Activates privilege escalation. become_user: apache # <- Changes the user with the desired privileges. + +# Stand alone playbook alternative, applies to all tasks + +- name: Example playbook + hosts: localhost + become: true # <- Activates privilege escalation. + become_user: apache # <- Changes the user with the desired privileges. + tasks: + - name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook 1 + hosts: localhost + become: true # <- Activates privilege escalation. + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +--- +- name: Example playbook 2 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +# task.yml +- name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started + become_user: apache # <- Does not change the user because "become: true" is not set. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook 1 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +--- +- name: Example playbook 2 + hosts: localhost + tasks: + - name: Include a task file + ansible.builtin.include_tasks: tasks.yml +``` + +```yaml +# task.yml +- name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started + become: true # <- Activates privilege escalation. + become_user: apache # <- Does not change the user because "become: true" is not set. ``` diff --git a/src/ansiblelint/rules/partial_become.py b/src/ansiblelint/rules/partial_become.py index 85101b511b..9c7edf19bf 100644 --- a/src/ansiblelint/rules/partial_become.py +++ b/src/ansiblelint/rules/partial_become.py @@ -23,11 +23,13 @@ import sys from typing import TYPE_CHECKING, Any +from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.constants import LINE_NUMBER_KEY from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: - from ruamel.yaml.comments import CommentedMap, CommentedSeq + from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable @@ -35,80 +37,205 @@ class BecomeUserWithoutBecomeRule(AnsibleLintRule, TransformMixin): - """become_user requires become to work as expected.""" + """``become_user`` should have a cooresponding ``become`` at the play or task level.""" id = "partial-become" - description = "``become_user`` without ``become`` will not actually change user" + description = "``become_user`` should have a cooresponding ``become`` at the play or task level." severity = "VERY_HIGH" tags = ["unpredictability"] version_added = "historic" - def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: - if ( - file.kind == "playbook" - and data.get("become_user") - and not data.get("become") - ): - return [ - self.create_matcherror( + def matchplay(self: BecomeUserWithoutBecomeRule, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Match become_user without become in play. + + :param file: The file to lint. + :param data: The data to lint (play) + :returns: A list of errors. + """ + if file.kind != "playbook": + return [] + errors = [] + partial = "become_user" in data and "become" not in data + if partial: + error = self.create_matcherror( message=self.shortdesc, filename=file, tag=f"{self.id}[play]", lineno=data[LINE_NUMBER_KEY], - ), - ] - return [] + ) + errors.append(error) + return errors + def matchtask( - self, + self: BecomeUserWithoutBecomeRule, task: Task, file: Lintable | None = None, ) -> list[MatchError]: - if task.get("become_user") and not task.get("become"): - return [ - self.create_matcherror( + """Match become_user without become in task. + + :param task: The task to lint. + :param file: The file to lint. + :returns: A list of errors. + """ + data = task.normalized_task + errors = [] + partial = "become_user" in data and "become" not in data + if partial: + error = self.create_matcherror( message=self.shortdesc, filename=file, tag=f"{self.id}[task]", lineno=task[LINE_NUMBER_KEY], - ), - ] - return [] + ) + errors.append(error) + return errors + + def _dive(self: BecomeUserWithoutBecomeRule, data: CommentedSeq): + """Dive into the data and yield each item. + + :param data: The data to dive into. + :yield: Each item in the data. + """ + for item in data: + for nested in ("block", "rescue", "always"): + if nested in item: + yield from self._dive(item[nested]) + yield item def transform( - self, + self: BecomeUserWithoutBecomeRule, match: MatchError, lintable: Lintable, data: CommentedMap | CommentedSeq | str, ) -> None: - if match.tag in ("partial-become[play]", "partial-become[task]"): - target_task = self.seek(match.yaml_path, data) - for _ in range(len(target_task)): - k, v = target_task.popitem(False) - if k == "become_user": - target_task["become"] = True - target_task[k] = v + """Transform the data. + + :param match: The match to transform. + :param lintable: The file to transform. + :param data: The data to transform. + """ + if not isinstance(data, CommentedSeq): + return + + obj = self.seek(match.yaml_path, data) + if "become" in obj and "become_user" in obj: + match.fixed = True + return + if "become" not in obj and "become_user" not in obj: + match.fixed = True + return + + self._transform_plays(plays=data) + + if "become" in obj and "become_user" in obj: + match.fixed = True + return + if "become" not in obj and "become_user" not in obj: match.fixed = True + return + + + def is_ineligible_for_transform(self: BecomeUserWithoutBecomeRule, data: CommentedMap) -> bool: + """Check if the data is eligible for transformation. + + :param data: The data to check. + :returns: True if ineligible, False otherwise. + """ + if any("include" in key for key in data): + return True + if "notify" in data: + return True + return False + + def _transform_plays(self, plays: CommentedSeq) -> None: + """Transform the plays. + + :param plays: The plays to transform. + """ + for play in plays: + self._transform_play(play=play) + + def _transform_play(self, play: CommentedMap) -> None: + """Transform the play. + + :param play: The play to transform. + """ + # Ensure we have no includes in this play + task_groups = ("tasks", "pre_tasks", "post_tasks", "handlers") + for task_group in task_groups: + tasks = self._dive(play.get(task_group, [])) + for task in tasks: + if self.is_ineligible_for_transform(task): + return + remove_play_become_user = False + for task_group in task_groups: + tasks = self._dive(play.get(task_group, [])) + for task in tasks: + bit = "become" in task + buit = "become_user" in task + bip = "become" in play + buip = "become_user" in play + if bit and not buit and buip: + # Preserve the end comment if become is the last key + comment = None + if list(task.keys())[-1] == "become" and "become" in task.ca.items: + comment = task.ca.items.pop("become") + become_index = list(task.keys()).index("become") + task.insert(become_index + 1, "become_user", play["become_user"]) + if comment: + self._attach_comment_end(task, comment) + remove_play_become_user = True + if buit and not bit and bip: + become_user_index = list(task.keys()).index("become_user") + task.insert(become_user_index, "become", play["become"]) + if buit and not bit and not bip: + # Preserve the end comment if become_user is the last key + comment = None + if list(task.keys())[-1] == "become_user" and "become_user" in task.ca.items: + comment = task.ca.items.pop("become_user") + task.pop("become_user") + if comment: + self._attach_comment_end(task, comment) + if remove_play_become_user: + del play["become_user"] + + def _attach_comment_end(self, obj: CommentedMap | CommentedSeq, comment: Any) -> None: + """Attach a comment to the end of the object. + :param obj: The object to attach the comment to. + :param comment: The comment to attach. + """ + if isinstance(obj, CommentedMap): + last = list(obj.keys())[-1] + if not isinstance(obj[last], (CommentedSeq, CommentedMap)): + obj.ca.items[last] = comment + return + self._attach_comment_end(obj[last], comment) + elif isinstance(obj, CommentedSeq): + if not isinstance(obj[-1], (CommentedSeq, CommentedMap)): + obj.ca.items[len(obj)] = comment + return + self._attach_comment_end(obj[-1], comment) # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports - def test_partial_become_positive() -> None: - """Positive test for partial-become.""" + def test_partial_become_pass() -> None: + """No errors found for partial-become.""" collection = RulesCollection() collection.register(BecomeUserWithoutBecomeRule()) success = "examples/playbooks/rule-partial-become-without-become-pass.yml" good_runner = Runner(success, rules=collection) assert [] == good_runner.run() - def test_partial_become_negative() -> None: - """Negative test for partial-become.""" + def test_partial_become_fail() -> None: + """Errors found for partial-become.""" collection = RulesCollection() collection.register(BecomeUserWithoutBecomeRule()) failure = "examples/playbooks/rule-partial-become-without-become-fail.yml" bad_runner = Runner(failure, rules=collection) errs = bad_runner.run() - assert len(errs) == 3 + assert len(errs) == 4 diff --git a/test/test_transformer.py b/test/test_transformer.py index 4950175624..0b48ba05ff 100644 --- a/test/test_transformer.py +++ b/test/test_transformer.py @@ -104,7 +104,7 @@ def fixture_runner_result( ), pytest.param( "examples/playbooks/transform-partial-become.yml", - 8, + 4, True, id="partial_become", ), From 5d43cc615fad9756d7af43f5e1d82a00fc136ae1 Mon Sep 17 00:00:00 2001 From: shatakshiiii Date: Thu, 7 Sep 2023 15:22:39 +0530 Subject: [PATCH 4/7] pre-commit fixes --- .../main.yml | 0 .../transform-partial-become.transformed.yml | 2 +- .../playbooks/transform-partial-become.yml | 2 +- src/ansiblelint/rules/partial_become.py | 67 +++++++++++-------- 4 files changed, 42 insertions(+), 29 deletions(-) rename examples/playbooks/tasks/{patial_become.yml => partial_become.yml}/main.yml (100%) diff --git a/examples/playbooks/tasks/patial_become.yml/main.yml b/examples/playbooks/tasks/partial_become.yml/main.yml similarity index 100% rename from examples/playbooks/tasks/patial_become.yml/main.yml rename to examples/playbooks/tasks/partial_become.yml/main.yml diff --git a/examples/playbooks/transform-partial-become.transformed.yml b/examples/playbooks/transform-partial-become.transformed.yml index 066c39ad7b..31d2a158c4 100644 --- a/examples/playbooks/transform-partial-become.transformed.yml +++ b/examples/playbooks/transform-partial-become.transformed.yml @@ -39,7 +39,7 @@ become_user: root # The play has become_user but has an include -# this is unfixable, the include could be called from multiple playbooks +# this is not fixable, the include could be called from multiple playbooks - name: Play 4 hosts: localhost become_user: root diff --git a/examples/playbooks/transform-partial-become.yml b/examples/playbooks/transform-partial-become.yml index b1e4f39d13..079d1a0160 100644 --- a/examples/playbooks/transform-partial-become.yml +++ b/examples/playbooks/transform-partial-become.yml @@ -39,7 +39,7 @@ become_user: root # The play has become_user but has an include -# this is unfixable, the include could be called from multiple playbooks +# this is not fixable, the include could be called from multiple playbooks - name: Play 4 hosts: localhost become_user: root diff --git a/src/ansiblelint/rules/partial_become.py b/src/ansiblelint/rules/partial_become.py index 9c7edf19bf..62af6fd98c 100644 --- a/src/ansiblelint/rules/partial_become.py +++ b/src/ansiblelint/rules/partial_become.py @@ -29,7 +29,7 @@ from ansiblelint.rules import AnsibleLintRule, TransformMixin if TYPE_CHECKING: - + from collections.abc import Iterator from ansiblelint.errors import MatchError from ansiblelint.file_utils import Lintable @@ -37,15 +37,19 @@ class BecomeUserWithoutBecomeRule(AnsibleLintRule, TransformMixin): - """``become_user`` should have a cooresponding ``become`` at the play or task level.""" + """``become_user`` should have a corresponding ``become`` at the play or task level.""" id = "partial-become" - description = "``become_user`` should have a cooresponding ``become`` at the play or task level." + description = "``become_user`` should have a corresponding ``become`` at the play or task level." severity = "VERY_HIGH" tags = ["unpredictability"] version_added = "historic" - def matchplay(self: BecomeUserWithoutBecomeRule, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + def matchplay( + self: BecomeUserWithoutBecomeRule, + file: Lintable, + data: dict[str, Any], + ) -> list[MatchError]: """Match become_user without become in play. :param file: The file to lint. @@ -58,15 +62,14 @@ def matchplay(self: BecomeUserWithoutBecomeRule, file: Lintable, data: dict[str, partial = "become_user" in data and "become" not in data if partial: error = self.create_matcherror( - message=self.shortdesc, - filename=file, - tag=f"{self.id}[play]", - lineno=data[LINE_NUMBER_KEY], - ) + message=self.shortdesc, + filename=file, + tag=f"{self.id}[play]", + lineno=data[LINE_NUMBER_KEY], + ) errors.append(error) return errors - def matchtask( self: BecomeUserWithoutBecomeRule, task: Task, @@ -83,15 +86,15 @@ def matchtask( partial = "become_user" in data and "become" not in data if partial: error = self.create_matcherror( - message=self.shortdesc, - filename=file, - tag=f"{self.id}[task]", - lineno=task[LINE_NUMBER_KEY], - ) + message=self.shortdesc, + filename=file, + tag=f"{self.id}[task]", + lineno=task[LINE_NUMBER_KEY], + ) errors.append(error) return errors - def _dive(self: BecomeUserWithoutBecomeRule, data: CommentedSeq): + def _dive(self: BecomeUserWithoutBecomeRule, data: CommentedSeq) -> Iterator[Any]: """Dive into the data and yield each item. :param data: The data to dive into. @@ -135,8 +138,10 @@ def transform( match.fixed = True return - - def is_ineligible_for_transform(self: BecomeUserWithoutBecomeRule, data: CommentedMap) -> bool: + def is_ineligible_for_transform( + self: BecomeUserWithoutBecomeRule, + data: CommentedMap, + ) -> bool: """Check if the data is eligible for transformation. :param data: The data to check. @@ -172,11 +177,11 @@ def _transform_play(self, play: CommentedMap) -> None: for task_group in task_groups: tasks = self._dive(play.get(task_group, [])) for task in tasks: - bit = "become" in task - buit = "become_user" in task - bip = "become" in play - buip = "become_user" in play - if bit and not buit and buip: + b_in_t = "become" in task + bu_in_t = "become_user" in task + b_in_p = "become" in play + bu_in_p = "become_user" in play + if b_in_t and not bu_in_t and bu_in_p: # Preserve the end comment if become is the last key comment = None if list(task.keys())[-1] == "become" and "become" in task.ca.items: @@ -186,13 +191,16 @@ def _transform_play(self, play: CommentedMap) -> None: if comment: self._attach_comment_end(task, comment) remove_play_become_user = True - if buit and not bit and bip: + if bu_in_t and not b_in_t and b_in_p: become_user_index = list(task.keys()).index("become_user") task.insert(become_user_index, "become", play["become"]) - if buit and not bit and not bip: + if bu_in_t and not b_in_t and not b_in_p: # Preserve the end comment if become_user is the last key comment = None - if list(task.keys())[-1] == "become_user" and "become_user" in task.ca.items: + if ( + list(task.keys())[-1] == "become_user" + and "become_user" in task.ca.items + ): comment = task.ca.items.pop("become_user") task.pop("become_user") if comment: @@ -200,7 +208,11 @@ def _transform_play(self, play: CommentedMap) -> None: if remove_play_become_user: del play["become_user"] - def _attach_comment_end(self, obj: CommentedMap | CommentedSeq, comment: Any) -> None: + def _attach_comment_end( + self, + obj: CommentedMap | CommentedSeq, + comment: Any, + ) -> None: """Attach a comment to the end of the object. :param obj: The object to attach the comment to. @@ -218,6 +230,7 @@ def _attach_comment_end(self, obj: CommentedMap | CommentedSeq, comment: Any) -> return self._attach_comment_end(obj[-1], comment) + # testing code to be loaded only with pytest or when executed the rule file if "pytest" in sys.modules: from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports From f8168a7c5e0501d39a44fa3481e426c8bc257400 Mon Sep 17 00:00:00 2001 From: shatakshiiii Date: Thu, 7 Sep 2023 15:46:04 +0530 Subject: [PATCH 5/7] fixes for tests --- examples/playbooks/rule-partial-become-without-become-pass.yml | 2 +- examples/playbooks/tasks/partial_become.yml/main.yml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/examples/playbooks/rule-partial-become-without-become-pass.yml b/examples/playbooks/rule-partial-become-without-become-pass.yml index 11a0d2be3d..c01b1410a1 100644 --- a/examples/playbooks/rule-partial-become-without-become-pass.yml +++ b/examples/playbooks/rule-partial-become-without-become-pass.yml @@ -5,7 +5,7 @@ become: true tasks: - - name: debug + - name: Debug ansible.builtin.debug: msg: hello diff --git a/examples/playbooks/tasks/partial_become.yml/main.yml b/examples/playbooks/tasks/partial_become.yml/main.yml index 622ed88338..c7f1980270 100644 --- a/examples/playbooks/tasks/partial_become.yml/main.yml +++ b/examples/playbooks/tasks/partial_become.yml/main.yml @@ -1,3 +1,4 @@ +--- - name: Included with partial become ansible.builtin.debug: msg: Included with partial become From ddbeabd2e5f8a7cc0ead437115f7187b6e85d542 Mon Sep 17 00:00:00 2001 From: shatakshiiii Date: Fri, 8 Sep 2023 13:29:38 +0530 Subject: [PATCH 6/7] more fixes for tests --- src/ansiblelint/rules/partial_become.md | 4 ++-- src/ansiblelint/rules/partial_become.py | 2 +- test/test_yaml_utils.py | 22 +++++++++++----------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/ansiblelint/rules/partial_become.md b/src/ansiblelint/rules/partial_become.md index 7ff20e583e..763cb67bc9 100644 --- a/src/ansiblelint/rules/partial_become.md +++ b/src/ansiblelint/rules/partial_become.md @@ -83,7 +83,7 @@ This rule can produce the following messages: ``` ```yaml -# task.yml +# tasks.yml - name: Start the httpd service as the apache user ansible.builtin.service: name: httpd @@ -112,7 +112,7 @@ This rule can produce the following messages: ``` ```yaml -# task.yml +# tasks.yml - name: Start the httpd service as the apache user ansible.builtin.service: name: httpd diff --git a/src/ansiblelint/rules/partial_become.py b/src/ansiblelint/rules/partial_become.py index 62af6fd98c..b8dc417052 100644 --- a/src/ansiblelint/rules/partial_become.py +++ b/src/ansiblelint/rules/partial_become.py @@ -251,4 +251,4 @@ def test_partial_become_fail() -> None: failure = "examples/playbooks/rule-partial-become-without-become-fail.yml" bad_runner = Runner(failure, rules=collection) errs = bad_runner.run() - assert len(errs) == 4 + assert len(errs) == 3 diff --git a/test/test_yaml_utils.py b/test/test_yaml_utils.py index ae3a2b68a1..326816e541 100644 --- a/test/test_yaml_utils.py +++ b/test/test_yaml_utils.py @@ -386,7 +386,7 @@ def fixture_ruamel_data(lintable: Lintable) -> CommentedMap | CommentedSeq: ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 10, + 12, [1], id="4_play_playbook-first_line_in_play_2", ), @@ -404,7 +404,7 @@ def fixture_ruamel_data(lintable: Lintable) -> CommentedMap | CommentedSeq: ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 19, + 21, [2], id="4_play_playbook-first_line_in_play_3", ), @@ -422,7 +422,7 @@ def fixture_ruamel_data(lintable: Lintable) -> CommentedMap | CommentedSeq: ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 29, + 31, [3], id="4_play_playbook-first_line_in_play_4", ), @@ -603,7 +603,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 7, + 8, [0, "tasks", 0], id="4_play_playbook-play_1_first_line_task_1", ), @@ -615,7 +615,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 10, + 13, [], id="4_play_playbook-play_2_line_before_tasks", ), @@ -627,7 +627,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 13, + 15, [1, "tasks", 0], id="4_play_playbook-play_2_first_line_task_1", ), @@ -645,7 +645,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 19, + 23, [], id="4_play_playbook-play_3_line_before_tasks", ), @@ -657,7 +657,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 23, + 25, [2, "tasks", 0], id="4_play_playbook-play_3_first_line_task_1", ), @@ -675,7 +675,7 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 29, + 33, [], id="4_play_playbook-play_4_line_before_tasks", ), @@ -687,13 +687,13 @@ def test_get_path_to_play( ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 34, + 35, [3, "tasks", 0], id="4_play_playbook-play_4_first_line_task_1", ), pytest.param( "examples/playbooks/rule-partial-become-without-become-pass.yml", - 35, + 39, [3, "tasks", 0], id="4_play_playbook-play_4_middle_line_task_1", ), From 6af5244a71347c355cd729e3eb7d9bc41c328705 Mon Sep 17 00:00:00 2001 From: shatakshiiii Date: Fri, 8 Sep 2023 13:58:46 +0530 Subject: [PATCH 7/7] update test count --- .github/workflows/tox.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index 9db6e9dcc6..9fe7b7a768 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -71,7 +71,7 @@ jobs: WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 821 + PYTEST_REQPASS: 822 steps: - name: Activate WSL1 if: "contains(matrix.shell, 'wsl')"