From 443a628374ea88d0d8e90fc31adde9e797e20e1f Mon Sep 17 00:00:00 2001 From: Navid Sassan Date: Fri, 18 Sep 2026 12:10:05 +0200 Subject: [PATCH 1/2] fix(plugins/bitwarden_item): prevent duplicate items from parallel runs Parallel Ansible workers each synced bw serve, found no matching item and created one, even for items that had existed for months. Two causes: - No coordination between processes: every worker synced, searched and created on its own, and the last one to write the cache file won. - bw serve answers list/object/items with success and no items for a few seconds after a sync, and 400 for an existing item in the same window (https://github.com/bitwarden/clients/issues/23283, reproduced with bw 2026.8.0 and 2026.9.0). The lookup and the module now hold a mutex (flock on a file next to the cache) while they sync, search and create, and re-read the cache once they hold it. It is not called a lock to keep it apart from locking the vault. An empty item list is asked for again 5 times, 2s apart, and then aborts instead of being taken as an empty vault. The API fallback of get_item_by_id() retries the same way. --- CHANGELOG.md | 2 + plugins/lookup/bitwarden_item.py | 8 + plugins/module_utils/bitwarden.py | 114 ++++++++++- plugins/modules/bitwarden_item.py | 141 +++++++------- .../plugins/lookup/test_bitwarden_item.py | 21 +- .../plugins/module_utils/test_bitwarden.py | 180 ++++++++++++++++++ .../plugins/modules/test_bitwarden_item.py | 23 ++- 7 files changed, 414 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f6df5d91..7c8f6c35b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +* **plugin:bitwarden_item, module:bitwarden_item**: A run against a vault that contains no items at all aborts instead of creating the first one, because `bw serve` briefly reports an empty vault after every sync ([bitwarden/clients#23283](https://github.com/bitwarden/clients/issues/23283)). * **role:repo_postgresql**: The PostgreSQL version repositories take precedence over the distribution's packages of the same name, so on RHEL 10 an install or update no longer switches a PostgreSQL server from the PGDG build to the AppStream build, which uses a different file layout. * **role:apache_httpd**: Responses of type `text/markdown` are compressed like HTML, so the Markdown versions of pages that CMSs such as Grav hand to AI agents no longer go out uncompressed. * **role:grav**: The README lists setting `session.secure` as a manual step behind a reverse proxy that terminates TLS, where Grav sends its session cookies without the `Secure` flag. @@ -55,6 +56,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +* **plugin:bitwarden_item, module:bitwarden_item**: Running against several hosts in parallel no longer creates duplicates of a Bitwarden item, whether the item is new or has existed for a long time, so the next run no longer aborts with "Found multiple Bitwarden items". * **role:wordpress**: The installation no longer aborts at `wp core download` when Ansible connects as `root` without privilege escalation. * **role:wordpress**: A second run no longer reports the `wp-config.php` constants, `wordpress__plugins` and `wordpress__theme` as changed. * **role:wordpress**: The WXR file in `wordpress__wxr_export` is imported when the role installs WordPress; the import never ran before. diff --git a/plugins/lookup/bitwarden_item.py b/plugins/lookup/bitwarden_item.py index df1994d8b..d0c985c67 100644 --- a/plugins/lookup/bitwarden_item.py +++ b/plugins/lookup/bitwarden_item.py @@ -24,6 +24,8 @@ - When I(name) is omitted, a title is generated automatically as C(hostname - purpose) (e.g. C(dbserver - MariaDB)) or just C(hostname) when no purpose is given. - Generated passwords use Python's C(secrets) module (cryptographically strong RNG), not the Bitwarden generator. This lifts the 128-character limit and allows arbitrary character sets, including hex. - Items are read from a local on-disk cache backed by C(bw serve). A cached C(bw sync) is performed at most every 60 seconds, so consecutive lookups in the same play do not hammer the API. + - Lookups on the same controller run one at a time, so hosts that are processed in parallel and need the same missing item create it only once. + - Right after a sync, C(bw serve) can report an empty vault for a few seconds (U(https://github.com/bitwarden/clients/issues/23283)). The plugin then asks again for about ten seconds and fails rather than treat every item as missing. A vault that really is empty needs one item created by hand first. notes: - Lookups are evaluated by the templating engine on the controller and have no notion of check mode, so a run with C(--check) creates a missing item for real. Set I(create) to C(false) to turn that into a failure. @@ -315,6 +317,12 @@ def run(self, terms, variables=None, **kwargs): bw = Bitwarden() + # one lookup at a time on this controller, so parallel workers do not all + # create the same missing item + with bw.mutex(): + return self._run_under_mutex(bw, terms) + + def _run_under_mutex(self, bw, terms): status = bw.status if status != 'unlocked': raise AnsibleError(bw.get_not_unlocked_message(status)) diff --git a/plugins/module_utils/bitwarden.py b/plugins/module_utils/bitwarden.py index 5c461ab4a..130ab0c17 100644 --- a/plugins/module_utils/bitwarden.py +++ b/plugins/module_utils/bitwarden.py @@ -10,6 +10,7 @@ __metaclass__ = type +import contextlib import copy import email.encoders import email.mime.application @@ -17,6 +18,7 @@ import email.mime.nonmultipart import email.parser import email.policy +import fcntl import json import mimetypes import os @@ -152,6 +154,16 @@ def prepare_multipart_no_base64(fields): CACHE_FILE = os.path.join(CACHE_DIR, 'lfops_bitwarden_cache.json') CACHE_VERSION = 2026032701 +# how long a process waits for another one to finish its sync, search and create +MUTEX_TIMEOUT = 300 + +# `bw serve` briefly reports an empty vault right after a sync, see +# https://github.com/bitwarden/clients/issues/23283 +# Verified against bw 2026.8.0 and 2026.9.0 on Rocky Linux 9: the list recovers +# within a few seconds, so ask again a few times before giving up. +EMPTY_LIST_RETRIES = 5 +EMPTY_LIST_RETRY_DELAY = 2 + class BitwardenException(Exception): pass @@ -163,8 +175,66 @@ class Bitwarden: def __init__(self, hostname='127.0.0.1', port=8087): self._base_url = f'http://{hostname}:{port}' self._cache = None + self._mutex_fd = None + self._load_cache() + + def _acquire_mutex(self, timeout=MUTEX_TIMEOUT): + """Take the mutex, waiting at most `timeout` seconds. Use mutex() instead.""" + if self._mutex_fd is not None: + return + mutex_file = f'{CACHE_FILE}.mutex' + try: + fd = os.open(mutex_file, os.O_RDWR | os.O_CREAT, 0o600) + except OSError as e: + raise BitwardenException( + f'Unable to open the mutex file {mutex_file}: {to_native(e)}' + ) from e + deadline = time.time() + timeout + while True: + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + break + except BlockingIOError: + if time.time() >= deadline: + os.close(fd) + raise BitwardenException( + f'Another process has held {mutex_file} for more than {timeout}s. ' + 'Check for a hanging Ansible run using the Bitwarden lookup or module' + ) from None + time.sleep(0.2) + self._mutex_fd = fd + display.vvv(f'lfbw - acquired mutex {mutex_file}') self._load_cache() + def _release_mutex(self): + if self._mutex_fd is None: + return + fcntl.flock(self._mutex_fd, fcntl.LOCK_UN) + os.close(self._mutex_fd) + self._mutex_fd = None + display.vvv('lfbw - released mutex') + + @contextlib.contextmanager + def mutex(self, timeout=MUTEX_TIMEOUT): + """Let only one process at a time sync, search and create. + + Ansible evaluates a lookup in one worker process per host. Without the mutex, + workers that all miss an item create it once each, and each one overwrites the + cache file with its own view of the vault. The cache is re-read once the mutex + is held, so a process sees what the previous holder synced or created. + + Hold it only for the duration of one lookup or module run. flock() belongs to + the open file, not to the process, so a second Bitwarden() in the same process, + for example the next evaluation of the same lookup, would otherwise wait for + the first one. This is also why the release must not be left to the end of the + process: Mitogen can run modules in an interpreter that stays alive. + """ + self._acquire_mutex(timeout) + try: + yield self + finally: + self._release_mutex() + def _api_call(self, url_path, method='GET', body=None, body_format='json'): url = f'{self._base_url}/{url_path}' @@ -306,12 +376,36 @@ def sync(self, force=False, interval=60): return display.vvv(f'lfbw - syncing vault (force={force})') self._api_call('sync', method='POST') - result = self._api_call('list/object/items') - self._cache['items'] = result['data']['data'] + self._cache['items'] = self._list_items() self._cache['sync_timestamp'] = time.time() display.vvv(f'lfbw - sync complete, cached {len(self._cache["items"])} items') self._save_cache() + def _list_items(self, retries=EMPTY_LIST_RETRIES, delay=EMPTY_LIST_RETRY_DELAY): + """Return all items of the vault. An empty list is not trusted. + + Right after a sync, `bw serve` answers `list/object/items` with `success: true` + and no items for a few seconds. Taken at face value, every item would look + missing and be created again, so ask again, and give up rather than accept an + empty vault. + """ + for attempt in range(retries + 1): + items = self._api_call('list/object/items')['data']['data'] + if items: + return items + if attempt < retries: + display.vvv( + f'lfbw - bw serve returned an empty item list, retrying in {delay}s' + ) + time.sleep(delay) + raise BitwardenException( + f'`bw serve` reported an empty vault {retries + 1} times in a row. It does ' + 'that for a few seconds after a sync ' + '(https://github.com/bitwarden/clients/issues/23283), so the list is not ' + 'trusted, since every item would look missing and be created again. If the ' + 'vault really is empty, create any item in it first' + ) + def get_items( self, name, @@ -384,7 +478,9 @@ def get_items( display.vvv(f'lfbw - found {len(matching_items)} matching item(s)') return matching_items - def get_item_by_id(self, item_id): + def get_item_by_id( + self, item_id, retries=EMPTY_LIST_RETRIES, delay=EMPTY_LIST_RETRY_DELAY + ): """Get an item by ID from Bitwarden. Looks in the cache first, then falls back to the API (the item may have been created externally). Returns the item; raises BitwardenException if the API does not know the ID. @@ -396,8 +492,16 @@ def get_item_by_id(self, item_id): return item # fallback to API if not found in cache (item could have been created externally) display.vvv('lfbw - item not in cache, falling back to API') - result = self._api_call(f'object/item/{item_id}') - return result['data'] + # in the same window after a sync in which `list/object/items` comes back empty, + # `bw serve` answers 400 for an existing item, see _list_items() + for attempt in range(retries + 1): + try: + return self._api_call(f'object/item/{item_id}')['data'] + except BitwardenException: + if attempt == retries: + raise + display.vvv(f'lfbw - item lookup failed, retrying in {delay}s') + time.sleep(delay) def generate( self, diff --git a/plugins/modules/bitwarden_item.py b/plugins/modules/bitwarden_item.py index 6345d9211..d9d40b0a6 100644 --- a/plugins/modules/bitwarden_item.py +++ b/plugins/modules/bitwarden_item.py @@ -23,6 +23,8 @@ - When I(name) is omitted, a title is generated automatically as C(hostname - purpose) (e.g. C(appsrv01 - MariaDB)) or just C(hostname) when no purpose is given. - On success, the module returns the full Bitwarden item object. C(username) and C(password) are additionally lifted to the top level so they can be addressed without going through the C(login) sub-dictionary. - Items are read from a local on-disk cache backed by C(bw serve). A cached C(bw sync) is performed at most every 60 seconds, so consecutive calls in the same play do not hammer the API. + - Module runs on the same host run one at a time, so hosts that are processed in parallel and need the same missing item create it only once. + - Right after a sync, C(bw serve) can report an empty vault for a few seconds (U(https://github.com/bitwarden/clients/issues/23283)). The module then asks again for about ten seconds and fails rather than treat every item as missing. A vault that really is empty needs one item created by hand first. notes: - Only login items (Bitwarden type 1) are managed. Cards, secure notes and identities are out of scope. @@ -344,83 +346,86 @@ def run_module(): username = module.params['username'] bw = Bitwarden() + # one module run at a time on this host, so parallel runs do not all create the + # same missing item + with bw.mutex(): + status = bw.status + if status != 'unlocked': + module.fail_json(msg=bw.get_not_unlocked_message(status)) - status = bw.status - if status != 'unlocked': - module.fail_json(msg=bw.get_not_unlocked_message(status)) + # to be sure we are up to date + bw.sync() - # to be sure we are up to date - bw.sync() - - changed = False - if item_id: - current_item = bw.get_item_by_id(item_id) - else: - name = Bitwarden.get_pretty_name(name, hostname, purpose) - current_items = bw.get_items( - name, username, folder_id, collection_id, organization_id - ) - - if len(current_items) > 1: - module.fail_json( - msg='Found multiple Bitwarden items with the same name/title and username, cannot decide which one to use. Aborting.' + changed = False + if item_id: + current_item = bw.get_item_by_id(item_id) + else: + name = Bitwarden.get_pretty_name(name, hostname, purpose) + current_items = bw.get_items( + name, username, folder_id, collection_id, organization_id ) - current_item = current_items[0] if current_items else None - - login_uris = bw.get_template_item_login_uri(uris) - login = bw.get_template_item_login(username, password, login_uris) - target_item = bw.get_template_item( - name, - login, - notes, - organization_id, - collection_id, - folder_id, - ) + if len(current_items) > 1: + module.fail_json( + msg='Found multiple Bitwarden items with the same name/title and username, cannot decide which one to use. Aborting.' + ) - # 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 and not module.check_mode: - result = bw.edit_item(updated_item, updated_item['id']) - elif changed: - result = updated_item - else: - result = current_item + current_item = current_items[0] if current_items else None + + login_uris = bw.get_template_item_login_uri(uris) + login = bw.get_template_item_login(username, password, login_uris) + target_item = bw.get_template_item( + name, + login, + notes, + organization_id, + collection_id, + folder_id, + ) - else: - # generate a new one - changed = True - result = target_item if module.check_mode else bw.create_item(target_item) + # 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 and not module.check_mode: + result = bw.edit_item(updated_item, updated_item['id']) + elif changed: + result = updated_item + else: + result = current_item - if attachments: - current_attachments = { - current_attachment['fileName'] - for current_attachment in result.get('attachments', []) - } - attachments_changed = False - for attachment in attachments: - if os.path.basename(attachment) not in current_attachments: - attachments_changed = True + else: + # generate a new one + changed = True + result = target_item if module.check_mode else bw.create_item(target_item) + + if attachments: + current_attachments = { + current_attachment['fileName'] + for current_attachment in result.get('attachments', []) + } + attachments_changed = False + for attachment in attachments: + if os.path.basename(attachment) not in current_attachments: + attachments_changed = True + 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 if not module.check_mode: - bw.add_attachment(result['id'], attachment) + result = bw.get_item_by_id(result['id']) + + result['changed'] = changed + # move username and password higher for easier access + result['username'] = result['login']['username'] + result['password'] = result['login']['password'] - if attachments_changed: - changed = True - # we need to fetch the item again, so that it also contains the newly added attachments - if not module.check_mode: - result = bw.get_item_by_id(result['id']) - - result['changed'] = changed - # move username and password higher for easier access - result['username'] = result['login']['username'] - result['password'] = result['login']['password'] module.exit_json(**result) diff --git a/tests/unit/plugins/lookup/test_bitwarden_item.py b/tests/unit/plugins/lookup/test_bitwarden_item.py index 413c32772..d8b1ccb27 100644 --- a/tests/unit/plugins/lookup/test_bitwarden_item.py +++ b/tests/unit/plugins/lookup/test_bitwarden_item.py @@ -22,6 +22,7 @@ __metaclass__ = type +import contextlib import os import unittest from typing import ClassVar @@ -48,9 +49,19 @@ class _FakeBitwarden: created_items: ClassVar[list] = [] vault_status = 'unlocked' + mutex_held = False + def __init__(self, *args, **kwargs): pass + @contextlib.contextmanager + def mutex(self, *args, **kwargs): + type(self).mutex_held = True + try: + yield self + finally: + type(self).mutex_held = False + @property def status(self): return type(self).vault_status @@ -89,7 +100,9 @@ def get_template_item( return {'login': login, 'name': name, 'notes': notes} def create_item(self, item): - type(self).created_items.append(item) + type(self).created_items.append( + {**item, 'created_under_mutex': self.mutex_held} + ) return item @staticmethod @@ -104,6 +117,7 @@ def setUp(self): _FakeBitwarden.items_by_search = [] _FakeBitwarden.item_by_id = None _FakeBitwarden.created_items = [] + _FakeBitwarden.mutex_held = False _FakeBitwarden.vault_status = 'unlocked' # a value leaking in from the caller's environment would flip the # default of the `create` option under the tests' feet @@ -160,6 +174,11 @@ def test_missing_item_is_created_by_default(self): self.assertEqual(result[0]['username'], 'dba') self.assertEqual(result[0]['password'], 'linuxfabrik') + def test_item_is_created_under_the_mutex_and_mutex_is_released(self): + self.lookup.run([{'name': 'host - db', 'username': 'dba'}]) + self.assertTrue(_FakeBitwarden.created_items[0]['created_under_mutex']) + self.assertFalse(_FakeBitwarden.mutex_held) + def test_missing_item_raises_when_creation_disabled(self): os.environ[CREATE_ENV_VAR] = 'false' lookup = _load_lookup() diff --git a/tests/unit/plugins/module_utils/test_bitwarden.py b/tests/unit/plugins/module_utils/test_bitwarden.py index 6a4be9c82..49fb391bf 100644 --- a/tests/unit/plugins/module_utils/test_bitwarden.py +++ b/tests/unit/plugins/module_utils/test_bitwarden.py @@ -19,10 +19,16 @@ __metaclass__ = type +import contextlib +import functools import importlib.util import io import json +import multiprocessing import os +import queue +import tempfile +import time import unittest from urllib.error import HTTPError @@ -222,5 +228,179 @@ def test_no_match_returns_empty(self): self.assertEqual(self.bw.get_items('does-not-exist', username='dba'), []) +_LOGIN_ITEM = { + 'id': 'abc', + 'type': 1, + 'name': 'host - db', + 'login': {'username': 'dba', 'password': 'linuxfabrik'}, + 'folderId': None, + 'collectionIds': [], + 'organizationId': None, +} + + +def _serve_in_order(*payloads): + """Fake open_url that answers each call with the next payload. + + A payload that is an exception instance is raised instead. + """ + remaining = list(payloads) + calls = [] + + def _open_url(url, *args, **kwargs): + calls.append(url) + payload = remaining.pop(0) + if isinstance(payload, Exception): + raise payload + return _FakeResponse(payload) + + return _open_url, calls + + +def _items(*items): + return {'success': True, 'data': {'object': 'list', 'data': list(items)}} + + +_SYNCED = {'success': True, 'data': {'object': 'message', 'title': 'Syncing complete.'}} + + +class TestEmptyListIsNotTrusted(unittest.TestCase): + """bw serve briefly reports an empty vault right after a sync (#23283).""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.bw = _make_bitwarden(os.path.join(self._tmp.name, 'cache.json')) + self._orig_open_url = bitwarden.open_url + + def tearDown(self): + bitwarden.open_url = self._orig_open_url + self._tmp.cleanup() + + def test_empty_list_is_asked_again(self): + bitwarden.open_url, calls = _serve_in_order( + _items(), _items(), _items(_LOGIN_ITEM) + ) + self.assertEqual(self.bw._list_items(delay=0), [_LOGIN_ITEM]) + self.assertEqual(len(calls), 3) + + def test_non_empty_list_is_not_asked_again(self): + bitwarden.open_url, calls = _serve_in_order(_items(_LOGIN_ITEM)) + self.assertEqual(self.bw._list_items(delay=0), [_LOGIN_ITEM]) + self.assertEqual(len(calls), 1) + + def test_persistently_empty_list_raises(self): + bitwarden.open_url, calls = _serve_in_order(*[_items()] * 3) + with self.assertRaises(bitwarden.BitwardenException) as ctx: + self.bw._list_items(retries=2, delay=0) + self.assertEqual(len(calls), 3) + self.assertIn('23283', str(ctx.exception)) + # AnsibleError appends ". ", see TestStatus + self.assertFalse(str(ctx.exception).endswith('.')) + + def test_sync_does_not_cache_an_empty_list(self): + bitwarden.open_url, _calls = _serve_in_order( + _SYNCED, _items(), _items(_LOGIN_ITEM) + ) + # no real delay between the attempts + self.bw._list_items = functools.partial( + bitwarden.Bitwarden._list_items, self.bw, delay=0 + ) + self.bw.sync(force=True) + self.assertEqual(self.bw.get_items('host - db', username='dba'), [_LOGIN_ITEM]) + + def test_item_by_id_is_asked_again(self): + self.bw._cache['items'] = [] + bitwarden.open_url, calls = _serve_in_order( + HTTPError('http://127.0.0.1:8087/object/item/abc', 400, 'Bad', {}, None), + {'success': True, 'data': _LOGIN_ITEM}, + ) + self.assertEqual(self.bw.get_item_by_id('abc', delay=0), _LOGIN_ITEM) + self.assertEqual(len(calls), 2) + + def test_unknown_item_by_id_still_raises(self): + self.bw._cache['items'] = [] + bitwarden.open_url, calls = _serve_in_order( + *[{'success': False, 'data': 'Not found.'}] * 3 + ) + with self.assertRaises(bitwarden.BitwardenException): + self.bw.get_item_by_id('abc', retries=2, delay=0) + self.assertEqual(len(calls), 3) + + +def _create_if_missing(cache_file, created): + """Worker for TestMutex: the lookup's search-then-create, run in its own process.""" + bitwarden.CACHE_FILE = cache_file + bw = bitwarden.Bitwarden() + with bw.mutex(): + if bw._cache['items'] is None: + bw._cache['items'] = [] + if not bw.get_items('host - db', username='dba'): + time.sleep(0.2) # the time a create takes, the window of the race + bw._cache['items'].append(_LOGIN_ITEM) + bw._save_cache() + created.put(os.getpid()) + + +class TestMutex(unittest.TestCase): + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.cache_file = os.path.join(self._tmp.name, 'cache.json') + + def tearDown(self): + self._tmp.cleanup() + + def test_parallel_processes_create_a_missing_item_once(self): + # fork, so the children inherit the module loaded by file path above + ctx = multiprocessing.get_context('fork') + created = ctx.Queue() + workers = [ + ctx.Process(target=_create_if_missing, args=(self.cache_file, created)) + for _ in range(8) + ] + for worker in workers: + worker.start() + for worker in workers: + worker.join(timeout=60) + self.assertEqual(worker.exitcode, 0) + creators = [] + with contextlib.suppress(queue.Empty): + while True: + creators.append(created.get(timeout=1)) + self.assertEqual(len(creators), 1) + + def test_mutex_times_out_while_another_holder_has_it(self): + holder = _make_bitwarden(self.cache_file) + waiter = bitwarden.Bitwarden() + + def _wait_for_the_mutex(): + with waiter.mutex(timeout=0.5): + pass + + with holder.mutex(): + self.assertRaises(bitwarden.BitwardenException, _wait_for_the_mutex) + # released, so the next one gets it + _wait_for_the_mutex() + + def test_mutex_is_released_when_the_body_raises(self): + # fail_json() in the module leaves the with block through SystemExit + first = _make_bitwarden(self.cache_file) + second = bitwarden.Bitwarden() + with self.assertRaises(SystemExit), first.mutex(): + raise SystemExit(1) + with second.mutex(timeout=0.5): + pass + + def test_mutex_reloads_the_cache(self): + first = _make_bitwarden(self.cache_file) + second = bitwarden.Bitwarden() + with first.mutex(): + first._cache['items'] = [_LOGIN_ITEM] + first._save_cache() + # `second` loaded the cache before `first` wrote it + self.assertIsNone(second._cache['items']) + with second.mutex(): + self.assertEqual(second._cache['items'], [_LOGIN_ITEM]) + + if __name__ == '__main__': unittest.main() diff --git a/tests/unit/plugins/modules/test_bitwarden_item.py b/tests/unit/plugins/modules/test_bitwarden_item.py index 62e444ac0..87af8dd98 100644 --- a/tests/unit/plugins/modules/test_bitwarden_item.py +++ b/tests/unit/plugins/modules/test_bitwarden_item.py @@ -19,6 +19,7 @@ __metaclass__ = type +import contextlib import copy import unittest import unittest.mock @@ -93,10 +94,19 @@ class _FakeBitwarden: edited: ClassVar[list] = [] created: ClassVar[list] = [] vault_status = 'unlocked' + mutex_held = False def __init__(self, *args, **kwargs): pass + @contextlib.contextmanager + def mutex(self, *args, **kwargs): + type(self).mutex_held = True + try: + yield self + finally: + type(self).mutex_held = False + @property def status(self): return type(self).vault_status @@ -153,7 +163,9 @@ def edit_item(self, item, item_id): return result def create_item(self, item): - type(self).created.append(copy.deepcopy(item)) + type(self).created.append( + {**copy.deepcopy(item), 'under_mutex': self.mutex_held} + ) result = copy.deepcopy(item) result['id'] = 'new-id' return result @@ -164,6 +176,7 @@ def setUp(self): _FakeBitwarden.items = [] _FakeBitwarden.edited = [] _FakeBitwarden.created = [] + _FakeBitwarden.mutex_held = False _FakeBitwarden.vault_status = 'unlocked' self._patchers = [ unittest.mock.patch.object(mod, 'Bitwarden', _FakeBitwarden), @@ -194,6 +207,14 @@ def test_vault_not_unlocked_fails(self): self.assertIn(status, ctx.exception.args[0]['msg']) self.assertEqual(_FakeBitwarden.created, []) + def test_item_is_created_under_the_mutex_and_mutex_is_released(self): + result = self._run( + {'name': 'host - db', 'username': 'dba', 'password': 'linuxfabrik'} + ) + self.assertTrue(result['changed']) + self.assertTrue(_FakeBitwarden.created[0]['under_mutex']) + self.assertFalse(_FakeBitwarden.mutex_held) + def test_check_mode_create_does_not_write(self): _FakeBitwarden.items = [] # nothing exists -> would create result = self._run( From e9ac6aee6a308cc09d60667de8193389612b2f46 Mon Sep 17 00:00:00 2001 From: Navid Sassan Date: Fri, 18 Sep 2026 12:37:41 +0200 Subject: [PATCH 2/2] fix(plugins/bitwarden_item): make the module work under Mitogen module_utils/bitwarden.py imports ansible.utils.display and falls back to a no-op display on ImportError, for module runs on a managed node without the controller's ansible package. Under Mitogen the import is served from the controller and succeeds, but loading ansible.constants then raises AnsibleError ("Missing base YAML definition file"), because the data files are not served along. That escaped the except clause, so every run of the bitwarden_item module failed with MODULE FAILURE, for example the grafana role's "store tokens to bitwarden" task. Catch any exception for the fallback. Verified with Mitogen 0.3.53 and ansible-core 2.16 on Rocky Linux 9: 8 hosts calling the module three times each run through, and the mutex from the previous commit is released between the calls. --- CHANGELOG.md | 1 + plugins/module_utils/bitwarden.py | 8 +++-- .../plugins/module_utils/test_bitwarden.py | 33 +++++++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c8f6c35b..e1297c429 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +* **module:bitwarden_item**: The module works with the Mitogen strategy, where it aborted with `MODULE FAILURE` on every run, for example when the `grafana` role stores its service account tokens. * **plugin:bitwarden_item, module:bitwarden_item**: Running against several hosts in parallel no longer creates duplicates of a Bitwarden item, whether the item is new or has existed for a long time, so the next run no longer aborts with "Found multiple Bitwarden items". * **role:wordpress**: The installation no longer aborts at `wp core download` when Ansible connects as `root` without privilege escalation. * **role:wordpress**: A second run no longer reports the `wp-config.php` constants, `wordpress__plugins` and `wordpress__theme` as changed. diff --git a/plugins/module_utils/bitwarden.py b/plugins/module_utils/bitwarden.py index 130ab0c17..e6ceda419 100644 --- a/plugins/module_utils/bitwarden.py +++ b/plugins/module_utils/bitwarden.py @@ -36,9 +36,13 @@ from ansible.utils.display import Display display = Display() -except ImportError: +except Exception: # When used from a module (not a lookup plugin), this code runs inside an AnsiballZ - # process on the remote host where ansible.utils.display is not available. + # process on the remote host where ansible.utils.display is not available, so the + # import raises ImportError. Under Mitogen the import itself succeeds, because + # Mitogen serves it from the controller, but loading ansible.constants then raises + # AnsibleError ("Missing base YAML definition file"), since the data files are not + # served along. Verified with Mitogen 0.3.53 and ansible-core 2.16 on Rocky Linux 9. class _NoopDisplay: def vvv(self, msg, **kwargs): pass diff --git a/tests/unit/plugins/module_utils/test_bitwarden.py b/tests/unit/plugins/module_utils/test_bitwarden.py index 49fb391bf..ec18e68b0 100644 --- a/tests/unit/plugins/module_utils/test_bitwarden.py +++ b/tests/unit/plugins/module_utils/test_bitwarden.py @@ -30,6 +30,7 @@ import tempfile import time import unittest +import unittest.mock from urllib.error import HTTPError _MODULE_PATH = os.path.join( @@ -65,6 +66,38 @@ def _make_bitwarden(tmp_path='/nonexistent/lfops_bw_test_cache.json'): return bitwarden.Bitwarden() +class TestDisplayFallback(unittest.TestCase): + """The module falls back to a no-op display where Ansible's cannot be loaded.""" + + def _load_with_failing_display_import(self, exception): + real_import = __import__ + + def _import(name, *args, **kwargs): + if name == 'ansible.utils.display': + raise exception + return real_import(name, *args, **kwargs) + + spec = importlib.util.spec_from_file_location( + 'bitwarden_fallback', _MODULE_PATH + ) + module = importlib.util.module_from_spec(spec) + with unittest.mock.patch('builtins.__import__', _import): + spec.loader.exec_module(module) + return module + + def test_import_error_falls_back(self): + # AnsiballZ on a managed node without the controller's ansible package + module = self._load_with_failing_display_import(ImportError('no display')) + self.assertEqual(type(module.display).__name__, '_NoopDisplay') + + def test_other_error_falls_back(self): + # Mitogen: the import is served, but ansible.constants cannot find base.yml + module = self._load_with_failing_display_import( + RuntimeError('Missing base YAML definition file (bad install?)') + ) + self.assertEqual(type(module.display).__name__, '_NoopDisplay') + + class TestGenerate(unittest.TestCase): def test_length_and_charset(self): bw = _make_bitwarden()