1Password / connect-sdk-python

Python SDK for 1Password Connect
https://developer.1password.com/docs/connect
MIT License
200 stars 30 forks source link

Example for onepasswordconnectsdk.load_dict is incorrect #97

Open kwilsonmg opened 6 months ago

kwilsonmg commented 6 months ago

Your environment

SDK Version: N/A

Connect Server Version: N/A

OS: N/A

Python Version: N/A

What happened?

I am admittedly unsure what category to file this under, but I have discovered an error in USAGE.md. The load_dict example is inaccurate and will result in errors unless each item has opvault defined. If run without the fix (as below, though with values filled in) you get the following Traceback (or similar). This traceback also explicitly states the problem, which my pull request resolves.

# example dict configuration for onepasswordconnectsdk.load_dict(connect_client, CONFIG)
CONFIG = {
    "server": {
        "opitem": "My database item",
        "opfield": "specific_section.hostname",
        "opvault": "some_vault_id",
    },
    "database": {
        "opitem": "My database item",
        "opfield": ".database",
    },
    "username": {
        "opitem": "My database item",
        "opfield": ".username",
    },
    "password": {
        "opitem": "My database item",
        "opfield": ".password",
    },
}

values_dict = onepasswordconnectsdk.load_dict(connect_client, CONFIG)
Traceback (most recent call last):
  File "/Users/kylewilson/Documents/1PasswordTest/passwordtest.py", line 96, in <module>
    values_dict = onepasswordconnectsdk.load_dict(connect_client, CONFIG)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kylewilson/Documents/1PasswordTest/.venv/lib/python3.11/site-packages/onepasswordconnectsdk/config.py", line 65, in load_dict
    item_vault = _vault_uuid_for_field(field=field, vault_tag=vault_tag)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kylewilson/Documents/1PasswordTest/.venv/lib/python3.11/site-packages/onepasswordconnectsdk/config.py", line 158, in _vault_uuid_for_field
    raise NoVaultSetForFieldException(
onepasswordconnectsdk.config.NoVaultSetForFieldException: There         is no vault for database field

What did you expect to happen?

It should be as follows. This actually functions when tested:

# example dict configuration for onepasswordconnectsdk.load_dict(connect_client, CONFIG)
CONFIG = {
    "server": {
        "opitem": "My database item",
        "opfield": "specific_section.hostname",
        "opvault": "some_vault_id",
    },
    "database": {
        "opitem": "My database item",
        "opfield": ".database",
        "opvault": "some_vault_id",
    },
    "username": {
        "opitem": "My database item",
        "opfield": ".username",
        "opvault": "some_vault_id",
    },
    "password": {
        "opitem": "My database item",
        "opfield": ".password",
        "opvault": "some_vault_id",
    },
}

values_dict = onepasswordconnectsdk.load_dict(connect_client, CONFIG)

Steps to reproduce

  1. Run the example as-is, though obviously filling in the opitem and opvault fields where already present. Do not add any other fields.

Notes & Logs

volodymyrZotov commented 6 months ago

Hi @kwilsonmg . Good catch! Thanks for raising this!

Looking closer into that, I see that the error is thrown as the vaultID is not provided. However if go down the flow it uses item: Item = client.get_item(parsed_item.item_title, parsed_item.vault_uuid), which can find an item by title/ID only, so the vault_uuid is optional here.

That makes me think that the fix for that might be slightly different than changing the USAGE.md file. Simply saying, not throw an error if the opvault value is not provided.

kwilsonmg commented 6 months ago

So are you saying that _vault_uuid_for_field should be redefined to not throw an error or that we should try/except the error when that function is called in load_dict? @volodymyrZotov

volodymyrZotov commented 6 months ago

I'd say we just don't need to throw an error from _vault_uuid_for_field as we should be able to find an item just by item_uuid.