Extract shared OnePasswordClient into fides.common.onepassword

[adamsachs]

Description Of Changes

Extract the duplicated 1Password SDK integration from test helpers into a shared, instance-based OnePasswordClient class in fides.common.onepassword. This enables multiple consumers (SaaS test secrets, seed profile resolution in fidesplus) to each instantiate their own client with different service account tokens and vault IDs.

The existing test helper module-level API (get_secrets, get_item_by_title, list_available_items) is preserved as thin wrappers so no callers need to change.

Code Changes

• src/fides/common/onepassword/__init__.py - New package, exports OnePasswordClient
• src/fides/common/onepassword/client.py - New instance-based client with lazy init, get_item(), get_secrets(), get_item_notes(), get_item_notes_json(), list_items(), plus sync wrappers
• tests/ops/test_helpers/onepassword_client.py - Refactored to thin wrappers around OnePasswordClient

Steps to Confirm

1. Verify from fides.common.onepassword import OnePasswordClient imports cleanly
2. Verify existing SaaS test fixtures that use onepassword_client.get_secrets() still work (the module-level API is preserved)
3. Verify OnePasswordClient can be instantiated with different tokens/vault IDs for independent consumers

Pre-Merge Checklist

• Issue requirements met
• All CI pipelines succeeded
• CHANGELOG.md updated
  • Updates unreleased work already in Changelog, no new entry necessary
• UX feedback:
  • No UX review needed
• Followup issues:
  • No followup issues
• Database migrations:
  • No migrations
• Documentation:
  • No documentation updates required

Downstream: fidesplus seed-secret-profiles depends on this PR

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ignored		Mar 19, 2026 2:08am
fides-privacy-center	Ignored		Mar 19, 2026 2:08am

[adamsachs]

/code-review

[claude]

The extraction is clean and the refactored test helper is a nice reduction in duplication. A few things to address before merging:

Critical

• _get_client and _ensure_mappings both have a check-then-act race condition that will cause duplicate authentication/listing calls when two coroutines call them concurrently. Since OnePasswordClient is now in fides.common and targeted at production use (fidesplus seed profiles), this should be protected with asyncio.Lock before it lands.

Suggestions

• The sync wrappers (get_secrets_sync, get_item_notes_json_sync) create a fresh event loop on every call but reuse self._client (the SDK Client object) across those loops. If the SDK holds loop-bound connections internally, subsequent calls will fail. Consider using asyncio.run() and resetting self._client between calls, or verifying the SDK handles this gracefully.
• Duplicate vault item titles are silently overwritten — a warning log would make this easier to diagnose.

Nice to have

• Unit tests for OnePasswordClient (mock the SDK Client).
• A comment on the dual-condition lookup in get_item_notes (purpose == "NOTES" vs id == "notesPlain").

[claude]

Critical: Race condition under concurrent async usage

Both _get_client and _ensure_mappings use a check-then-act pattern without any lock:

if self._client is None:
    self._client = await Client.authenticate(...)  # yields to event loop here

If two coroutines call _get_client() concurrently, both can pass the is None check before either sets self._client. The same applies to _ensure_mappings — both coroutines could pass if self._mappings_initialized: return and then both call client.items.list(...).

Fix with an asyncio.Lock:

def __init__(self, ...):
    ...
    self._client_lock = asyncio.Lock()
    self._mappings_lock = asyncio.Lock()

async def _get_client(self) -> Client:
    if self._client is None:
        async with self._client_lock:
            if self._client is None:  # double-checked locking
                self._client = await Client.authenticate(...)
    return self._client

The same pattern applies to _ensure_mappings.

[claude]

Suggestion: Reusing SDK client across event loops

Each call to get_secrets_sync creates a fresh asyncio event loop, but self._client (the underlying 1Password SDK Client) is cached after the first call. On subsequent calls the cached client — authenticated in a now-closed loop — is reused in a new loop.

Whether this works depends on the SDK's internals. If it uses aiohttp or a similar library that stores loop-specific connections, subsequent calls will fail with RuntimeError: Event loop is closed or similar.

Two options:

1. Don't cache self._client across sync wrapper calls (drop the lazy cache, authenticate per call). Expensive but safe.
2. Use asyncio.run() instead of new_event_loop(), and clear self._client in finally so each asyncio.run() call starts fresh.

asyncio.run() is also the modern, recommended replacement for the manual new_event_loop()/run_until_complete()/close() pattern (available since Python 3.7).

[claude]

Suggestion: Duplicate vault item titles silently drop one

If the vault contains two items with the same title, the second silently overwrites the first in _title_to_id. Since get_item later uses this mapping, one item becomes permanently inaccessible with no indication why.

Consider adding a warning:

if item.title in self._title_to_id:
    logger.warning(f"1PW: duplicate item title '{item.title}' in vault {self._vault_id}; overwriting")
self._title_to_id[item.title] = {"item_id": item.id, "category": item.category}

[claude]

Nice to have: Dual-condition lookup deserves a comment

The or field.get("id") == "notesPlain" fallback is non-obvious. A brief comment explaining why both checks exist would help future readers:

# Check "purpose" first (canonical); fall back to the well-known field ID
# used by 1Password for plain-text notes items.
if field.get("purpose") == "NOTES" or field.get("id") == "notesPlain":

[claude]

Nice to have: No unit tests for the new class

OnePasswordClient is a new production module in fides.common but there are no corresponding tests. The 1Password SDK Client is straightforward to mock — a few tests covering get_item (found / not found), get_secrets (filters empty fields), get_item_notes (purpose vs. id fallback), and get_item_notes_json (valid JSON / invalid JSON) would give good coverage without needing a real vault.