Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/fides/common/onepassword/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
"""1Password client utilities for retrieving secrets and items."""

from fides.common.onepassword.client import OnePasswordClient

__all__ = ["OnePasswordClient"]
199 changes: 199 additions & 0 deletions src/fides/common/onepassword/client.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
"""
Instance-based 1Password client for retrieving secrets and items.

Provides lazy client initialization and vault item lookup by title.
Each instance is parameterized by its own service account token and vault ID,
so multiple consumers (e.g. SaaS test secrets, seed profile resolution)
can coexist with different credentials.
"""

import asyncio
import json
from typing import Any, Dict, List, Optional

from loguru import logger
from onepassword.client import Client # type: ignore[import-untyped]


Comment on lines +1 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

class OnePasswordClient:
"""1Password SDK client with lazy initialization and title-based lookup."""

def __init__(
self,
service_account_token: str,
vault_id: str,
integration_name: str = "Fides",
integration_version: str = "v1.0.0",
):
self._token = service_account_token
self._vault_id = vault_id
self._integration_name = integration_name
self._integration_version = integration_version
self._client: Optional[Client] = None
self._title_to_id: Dict[str, Dict[str, str]] = {}
self._mappings_initialized = False

# ------------------------------------------------------------------
# Internal helpers
# ------------------------------------------------------------------

async def _get_client(self) -> Client:
"""Lazily authenticate and return the 1Password SDK client."""
if self._client is None:
self._client = await Client.authenticate(
auth=self._token,
integration_name=self._integration_name,
integration_version=self._integration_version,
)
return self._client
Comment on lines +40 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


async def _ensure_mappings(self) -> None:
"""Build the title→ID index for items in the configured vault."""
if self._mappings_initialized:
return

client = await self._get_client()
items = await client.items.list(vault_id=self._vault_id)

for item in items:
logger.debug(f"1PW: indexed item '{item.title}'")
self._title_to_id[item.title] = {
"item_id": item.id,
"category": item.category,
}
Comment on lines +58 to +63
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}


self._mappings_initialized = True
logger.info(
f"1PW: initialized mappings for {len(self._title_to_id)} items "
f"in vault {self._vault_id}"
)

@staticmethod
def _item_to_dict(item: Any) -> Dict[str, Any]:
"""Convert a 1Password SDK item object to a plain dict."""
item_dict: Dict[str, Any] = {
"id": item.id,
"title": item.title,
"category": item.category,
"fields": [],
}

if hasattr(item, "fields") and item.fields:
for field in item.fields:
item_dict["fields"].append(
{
"id": getattr(field, "id", ""),
"title": getattr(field, "title", ""),
"type": getattr(field, "field_type", ""),
"value": getattr(field, "value", ""),
"purpose": getattr(field, "purpose", ""),
}
)

return item_dict

# ------------------------------------------------------------------
# Public API — async
# ------------------------------------------------------------------

async def get_item(self, title: str) -> Optional[Dict[str, Any]]:
"""
Retrieve a 1Password item by its title.

Returns a dict with keys: id, title, category, fields (list of
dicts with id, title, type, value, purpose). Returns None if
the item is not found.
"""
await self._ensure_mappings()

if title not in self._title_to_id:
logger.warning(f"1PW: item '{title}' not found in vault {self._vault_id}")
return None

info = self._title_to_id[title]
client = await self._get_client()
item = await client.items.get(
item_id=info["item_id"],
vault_id=self._vault_id,
)

item_dict = self._item_to_dict(item)
logger.info(
f"1PW: retrieved item '{title}' with {len(item_dict['fields'])} fields"
)
return item_dict

async def get_secrets(self, title: str) -> Dict[str, str]:
"""
Get field name → value pairs from a 1Password item.

Filters out fields with empty titles or values.
"""
item = await self.get_item(title)
if not item:
return {}

return {
field["title"]: field["value"]
for field in item.get("fields", [])
if field.get("title") and field.get("value")
}

async def get_item_notes(self, title: str) -> Optional[str]:
"""
Get the notes (notesPlain) content from a 1Password item.

Returns the string content of the NOTES field, or None if the
item is not found or has no notes.
"""
item = await self.get_item(title)
if not item:
return None

for field in item.get("fields", []):
Comment on lines +150 to +153
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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":

if field.get("purpose") == "NOTES" or field.get("id") == "notesPlain":
return field.get("value")

return None

async def get_item_notes_json(self, title: str) -> Optional[Dict[str, Any]]:
"""
Get the notes content from a 1Password item, parsed as JSON.

Returns the parsed dict, or None if the item is not found,
has no notes, or the notes are not valid JSON.
"""
notes = await self.get_item_notes(title)
if notes is None:
return None

try:
return json.loads(notes)
except json.JSONDecodeError:
logger.error(f"1PW: notes for item '{title}' are not valid JSON")
return None

async def list_items(self) -> List[str]:
"""List all item titles in the configured vault."""
await self._ensure_mappings()
return list(self._title_to_id.keys())

# ------------------------------------------------------------------
# Public API — sync wrappers
# ------------------------------------------------------------------

def get_secrets_sync(self, title: str) -> Dict[str, str]:
"""Synchronous wrapper around :meth:`get_secrets`."""
loop = asyncio.new_event_loop()
try:
return loop.run_until_complete(self.get_secrets(title))
finally:
Comment on lines +184 to +190
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

loop.close()

def get_item_notes_json_sync(self, title: str) -> Optional[Dict[str, Any]]:
"""Synchronous wrapper around :meth:`get_item_notes_json`."""
loop = asyncio.new_event_loop()
try:
return loop.run_until_complete(self.get_item_notes_json(title))
finally:
loop.close()
Loading
Loading