diff --git a/CHANGELOG.md b/CHANGELOG.md index b990216f..6d79d89d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +* **plugin:bitwarden_item**: The module no longer writes to the Bitwarden vault when run in check mode (`--check`); it reports the would-be change instead. +* **plugin:bitwarden_item**: A run without `password` (the default `None`) no longer overwrites an existing item's password; the current password is preserved, matching the documented behavior. * **plugin:sqlite_query**: A failed query now fails the task instead of reporting success with the error text in `query_result`. Playbooks that relied on the previous silent success will now correctly fail. * **plugin:sqlite_query**: A `REGEXP` query against a column that contains NULL values no longer fails; a NULL value simply does not match. * **plugin:uptimerobot_\***: The modules no longer crash when the UptimeRobot API returns a non-list response for a list endpoint; the response is passed through instead. diff --git a/plugins/modules/bitwarden_item.py b/plugins/modules/bitwarden_item.py index 36e6ec13..e0f5b379 100644 --- a/plugins/modules/bitwarden_item.py +++ b/plugins/modules/bitwarden_item.py @@ -28,7 +28,7 @@ - Only login items (Bitwarden type 1) are managed. Cards, secure notes and identities are out of scope. - TOTP secrets are not managed; the C(totp) field is set to an empty string on creation. - I(uris) replaces the URI list on every run. Omitting it on an existing item causes the URI list to be cleared. - - I(password) defaults to C(None), which translates to "do not set a password". Use the C(bitwarden_item) lookup plugin to generate one. + - I(password) defaults to C(None), which leaves the password unmanaged - a new item is created without one, and an existing item keeps its current password. Use the C(bitwarden_item) lookup plugin to generate one. - Attachments are matched by basename only; this module assumes that an attachment with the same basename has the same content. Pre-existing attachments are kept; only missing ones are uploaded. - Organization, collection and folder IDs can be copied from the URL in the Bitwarden web vault. - The cache file lives in C($XDG_RUNTIME_DIR) (falling back to C(/tmp)) and is shared across this module and the C(bitwarden_item) lookup within the same controller session. @@ -80,7 +80,7 @@ required: False type: str password: - description: Password to set on the login item. C(None) (the default) leaves the password field unset; existing passwords on already-existing items are overwritten by every non-C(None) value. + description: Password to set on the login item. C(None) (the default) leaves the password unmanaged - a new item gets no password and an existing item keeps its current one. Any non-C(None) value is written on every run. required: False type: str purpose: @@ -367,18 +367,26 @@ def run_module(): collection_id, folder_id, ) + + # A None password means "do not manage the password": preserve the existing + # item's password instead of overwriting it with null. + if password is None and current_item and current_item.get('login'): + target_item['login']['password'] = current_item['login'].get('password') + if current_item: # check if changed, adjust if necessary changed, updated_item = diff_and_update(current_item, target_item) - if changed: + if changed and not module.check_mode: result = bw.edit_item(updated_item, updated_item['id']) + elif changed: + result = updated_item else: result = current_item else: # generate a new one changed = True - result = bw.create_item(target_item) + result = target_item if module.check_mode else bw.create_item(target_item) if attachments: current_attachments = set(current_attachment['fileName'] for current_attachment in result.get('attachments', [])) @@ -386,12 +394,14 @@ def run_module(): for attachment in attachments: if os.path.basename(attachment) not in current_attachments: attachments_changed = True - bw.add_attachment(result['id'], attachment) + if not module.check_mode: + bw.add_attachment(result['id'], attachment) if attachments_changed: changed = True # we need to fetch the item again, so that it also contains the newly added attachments - result = bw.get_item_by_id(result['id']) + if not module.check_mode: + result = bw.get_item_by_id(result['id']) result['changed'] = changed # move username and password higher for easier access diff --git a/tests/unit/plugins/modules/test_bitwarden_item.py b/tests/unit/plugins/modules/test_bitwarden_item.py index 3fbba453..0089415d 100644 --- a/tests/unit/plugins/modules/test_bitwarden_item.py +++ b/tests/unit/plugins/modules/test_bitwarden_item.py @@ -6,10 +6,12 @@ # https://www.linuxfabrik.ch/ # License: The Unlicense, see LICENSE file. -"""Unit tests for the bitwarden_item module's pure diff helper. +"""Unit tests for the bitwarden_item module. -The module itself runs via AnsiballZ, but `diff_and_update` is a plain -function and is tested in isolation. The collection import is wired up by +`diff_and_update` is a plain function tested in isolation. The main() +behavior (check_mode must not write, a None password must not overwrite +an existing one) is tested with a fake Bitwarden client and the shared +ansible module harness. The collection import is wired up by tests/conftest.py. """ @@ -17,8 +19,13 @@ __metaclass__ = type +import copy import unittest +import unittest.mock +import ansible_harness + +from ansible_collections.linuxfabrik.lfops.plugins.modules import bitwarden_item as mod from ansible_collections.linuxfabrik.lfops.plugins.modules.bitwarden_item import diff_and_update @@ -62,5 +69,130 @@ def test_nested_dict_no_change(self): self.assertFalse(changed) +_EXISTING_ITEM = { + 'id': 'abc', + 'name': 'host - db', + 'login': {'username': 'dba', 'password': 'linuxfabrik-existing', 'totp': '', 'uris': []}, + 'notes': 'Generated by Ansible.', + 'organizationId': None, + 'collectionIds': None, + 'folderId': None, +} + + +class _FakeBitwarden: + """Stand-in for the Bitwarden client; records writes instead of doing them.""" + + items = [] + edited = [] + created = [] + + def __init__(self, *args, **kwargs): + pass + + @property + def is_unlocked(self): + return True + + def sync(self, *args, **kwargs): + pass + + def get_items(self, *args, **kwargs): + return [copy.deepcopy(i) for i in type(self).items] + + def get_item_by_id(self, item_id): + return None + + def get_template_item_login_uri(self, uris): + return list(uris or []) + + def get_template_item_login(self, username=None, password=None, login_uris=None): + return {'username': username, 'password': password, 'totp': '', 'uris': login_uris or []} + + def get_template_item(self, name, login=None, notes=None, organization_id=None, + collection_ids=None, folder_id=None): + return { + 'name': name, 'login': login, 'notes': notes, + 'organizationId': organization_id, 'collectionIds': collection_ids, + 'folderId': folder_id, + } + + @staticmethod + def get_pretty_name(name, hostname=None, purpose=None): + return name or hostname + + def edit_item(self, item, item_id): + type(self).edited.append((copy.deepcopy(item), item_id)) + result = copy.deepcopy(item) + result['id'] = item_id + return result + + def create_item(self, item): + type(self).created.append(copy.deepcopy(item)) + result = copy.deepcopy(item) + result['id'] = 'new-id' + return result + + +class TestMain(unittest.TestCase): + + def setUp(self): + _FakeBitwarden.items = [] + _FakeBitwarden.edited = [] + _FakeBitwarden.created = [] + self._patchers = [ + unittest.mock.patch.object(mod, 'Bitwarden', _FakeBitwarden), + ansible_harness.patch_module(), + ] + for p in self._patchers: + p.start() + + def tearDown(self): + for p in reversed(self._patchers): + p.stop() + + def _run(self, args): + ansible_harness.set_module_args(args) + try: + mod.run_module() + except ansible_harness.AnsibleExitJson as exc: + return exc.args[0] + raise AssertionError('module did not call exit_json') + + def test_check_mode_create_does_not_write(self): + _FakeBitwarden.items = [] # nothing exists -> would create + result = self._run({ + 'name': 'host - db', 'username': 'dba', 'password': 'linuxfabrik-new', + '_ansible_check_mode': True, + }) + self.assertTrue(result['changed']) + self.assertEqual(_FakeBitwarden.created, []) # no write in check mode + + def test_check_mode_edit_does_not_write(self): + _FakeBitwarden.items = [_EXISTING_ITEM] + result = self._run({ + 'name': 'host - db', 'username': 'dba', 'password': 'a-different-password', + '_ansible_check_mode': True, + }) + self.assertTrue(result['changed']) + self.assertEqual(_FakeBitwarden.edited, []) # no write in check mode + + def test_none_password_does_not_overwrite(self): + _FakeBitwarden.items = [_EXISTING_ITEM] + # password omitted entirely -> must preserve the existing one, no change + result = self._run({'name': 'host - db', 'username': 'dba'}) + self.assertFalse(result['changed']) + self.assertEqual(_FakeBitwarden.edited, []) + self.assertEqual(result['password'], 'linuxfabrik-existing') + + def test_changed_password_writes_when_not_check_mode(self): + _FakeBitwarden.items = [_EXISTING_ITEM] + result = self._run({ + 'name': 'host - db', 'username': 'dba', 'password': 'a-different-password', + }) + self.assertTrue(result['changed']) + self.assertEqual(len(_FakeBitwarden.edited), 1) + + if __name__ == '__main__': unittest.main()