Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example for onepasswordconnectsdk.load_dict is incorrect #97

Open
kwilsonmg opened this issue Mar 5, 2024 · 3 comments
Open

Example for onepasswordconnectsdk.load_dict is incorrect #97

kwilsonmg opened this issue Mar 5, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@kwilsonmg
Copy link

kwilsonmg commented Mar 5, 2024

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants