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.

Sign up for GitHub

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

Jump to bottom

onepassword - Support v2 #4728

Merged
merged 43 commits into from
Nov 6, 2022
Merged

onepassword - Support v2 #4728

merged 43 commits into from
Nov 6, 2022

Conversation

samdoran
Copy link
Contributor

SUMMARY

Fixes #4396.
Closes #4415.

The interface and data schema for op version two changed significantly. Create a base class defining the common interface then define version specific classes.

The appropriate class will be used based on the discovered op version.

The lookups/module will fail gracefully for unsupported versions.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/lookup/onepassword.py

ADDITIONAL INFORMATION

Still a work in progress. Open to feedback.

@ansibullbot
Copy link
Collaborator

cc @azenk @scottsb
click here for bot help

@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug lookup lookup plugin needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI plugins plugin (any type) and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels May 24, 2022
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels May 25, 2022
@samdoran
Copy link
Contributor Author

Still working on this, just as time allows.

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jun 8, 2022
@samdoran samdoran force-pushed the issue/4396-1p-cli-v2 branch from cc97ee2 to d4aae5c Compare June 21, 2022 17:49
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jun 21, 2022
@samdoran
Copy link
Contributor Author

Making some good progress. It's mostly working (only the full login is not). I need to do some more polishing and update the tests.

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jul 1, 2022
@samdoran samdoran mentioned this pull request Jul 6, 2022
Closed
@samdoran samdoran force-pushed the issue/4396-1p-cli-v2 branch from d4aae5c to 17a0cb0 Compare July 8, 2022 22:10
@ansibullbot ansibullbot added tests tests unit tests/unit and removed stale_ci CI is older than 7 days, rerun before merging labels Jul 8, 2022
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jul 8, 2022
@samdoran samdoran force-pushed the issue/4396-1p-cli-v2 branch from 17a0cb0 to 5cdb3cc Compare July 12, 2022 20:01
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Jul 12, 2022
@samdoran samdoran force-pushed the issue/4396-1p-cli-v2 branch from 82e66a1 to c34b332 Compare July 20, 2022 22:00
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jul 20, 2022
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added the new_plugin New plugin label Jul 22, 2022
@ansibullbot

This comment was marked as outdated.

Sign in to view
@samdoran
Move common classes back to lookup
152c25f 
The plugin relies on AnsibleLookupError() quite a bit which is not available
in module code.

Remove use of display for errors since section isn’t actually deprecated.
@samdoran samdoran marked this pull request as ready for review November 4, 2022 19:32
@samdoran
Properly handle sections
1f96628 
Still room for improvement, but this is at least a start.
@samdoran samdoran force-pushed the issue/4396-1p-cli-v2 branch from 037cb13 to 1f96628 Compare November 4, 2022 19:34
@ansibullbot ansibullbot removed the WIP Work in progress label Nov 4, 2022
@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 4, 2022
@ansibullbot

This comment was marked as outdated.

Sign in to view
@samdoran samdoran force-pushed the issue/4396-1p-cli-v2 branch from f7406da to 0ede3ac Compare November 4, 2022 20:08
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 4, 2022
@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 4, 2022
@samdoran
Copy link
Contributor Author

samdoran commented Nov 4, 2022

This is good to merge now.

felixfontein
felixfontein approved these changes Nov 4, 2022
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good to me (I only glanced over it though :) ). Assuming nobody complains until Sunday I'll merge.

@samdoran
Copy link
Contributor Author

samdoran commented Nov 4, 2022

Thanks for all the feedback and patience, everyone.

russoz pushed a commit to russoz-ansible/community.general that referenced this pull request Nov 5, 2022
@felixfontein @russoz
Lookups: use Ansible's config manager whenever possible (ansible-coll…
144fb77 
…ections#5440)

* Start using Ansible's config manager to handle options.

* Docs improvements.

* Fix documentation, make options actual lookup options.

* The cyberarkpassword lookup does too strange things.

* The onepassword lookups are converted in ansible-collections#4728, let's not interfere.

* Improve docs.

* Skip shelvefile as well.

* Convert lmdb_kv.

* Convert and fix credstash.

* Convert manifold.

* Drop chef_databag.

* Convert dig.

* Update examples.

* Forgot the most important part.

* Fix lmdb_kv docs.

* Python 2.6 compatibility.

* Convert AnsibleUnicode to str.

* Load lookup with lookup loader.

* Fix environment handling and error message checking.

* Improve docs formatting.
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Nov 6, 2022
@felixfontein felixfontein merged commit be0b5e5 into ansible-collections:main Nov 6, 2022
@felixfontein
Copy link
Collaborator

@samdoran thanks a lot for your contribution!
@ahussey-redhat @codello @markuman thanks for reviewing and giving feedback!

bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
@felixfontein
Lookups: use Ansible's config manager whenever possible (ansible-coll…
5694a0e 
…ections#5440)

* Start using Ansible's config manager to handle options.

* Docs improvements.

* Fix documentation, make options actual lookup options.

* The cyberarkpassword lookup does too strange things.

* The onepassword lookups are converted in ansible-collections#4728, let's not interfere.

* Improve docs.

* Skip shelvefile as well.

* Convert lmdb_kv.

* Convert and fix credstash.

* Convert manifold.

* Drop chef_databag.

* Convert dig.

* Update examples.

* Forgot the most important part.

* Fix lmdb_kv docs.

* Python 2.6 compatibility.

* Convert AnsibleUnicode to str.

* Load lookup with lookup loader.

* Fix environment handling and error message checking.

* Improve docs formatting.
bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
@felixfontein
Lookups: use Ansible's config manager whenever possible (ansible-coll…
e2265ec 
…ections#5440)

* Start using Ansible's config manager to handle options.

* Docs improvements.

* Fix documentation, make options actual lookup options.

* The cyberarkpassword lookup does too strange things.

* The onepassword lookups are converted in ansible-collections#4728, let's not interfere.

* Improve docs.

* Skip shelvefile as well.

* Convert lmdb_kv.

* Convert and fix credstash.

* Convert manifold.

* Drop chef_databag.

* Convert dig.

* Update examples.

* Forgot the most important part.

* Fix lmdb_kv docs.

* Python 2.6 compatibility.

* Convert AnsibleUnicode to str.

* Load lookup with lookup loader.

* Fix environment handling and error message checking.

* Improve docs formatting.
@samdoran samdoran deleted the issue/4396-1p-cli-v2 branch November 7, 2022 14:37
russoz pushed a commit to russoz-ansible/community.general that referenced this pull request Nov 10, 2022
@felixfontein @russoz
Lookups: use Ansible's config manager whenever possible (ansible-coll…
7180b73 
…ections#5440)

* Start using Ansible's config manager to handle options.

* Docs improvements.

* Fix documentation, make options actual lookup options.

* The cyberarkpassword lookup does too strange things.

* The onepassword lookups are converted in ansible-collections#4728, let's not interfere.

* Improve docs.

* Skip shelvefile as well.

* Convert lmdb_kv.

* Convert and fix credstash.

* Convert manifold.

* Drop chef_databag.

* Convert dig.

* Update examples.

* Forgot the most important part.

* Fix lmdb_kv docs.

* Python 2.6 compatibility.

* Convert AnsibleUnicode to str.

* Load lookup with lookup loader.

* Fix environment handling and error message checking.

* Improve docs formatting.
russoz pushed a commit to russoz-ansible/community.general that referenced this pull request Nov 10, 2022
@samdoran @russoz
onepassword - Support v2 (ansible-collections#4728)
4099a60 
* Begin building out separate classes to support  different op cli versions

Create separet base classes for each major version.
Define the main interface in the base class.
Create methods for getting the current version and instantiating the
appropriate class based on the found version.

* First pass at mostly working CLI version classes

* Correct mismathched parameters

* Update _run() method to allow updating enviroment

This allows passing in the app secret as an env var, which is more
secure than using a command line arg.

* Continuing to improve the interface

* Tear existing tests down to the studs

These tests were based off of the LastPass unit tests. I’m going to
just start from scratch given the new plugin code is vastly diffenent.

* Fix sanity test

* CLI config file path can be None

* Improve required param checking

- only report missing params
- use proper grammer based on number of missing params

* Change assert_logged_in() method return value

Return a boolean value indicating whether or not account is signed in

* Improve full login for v2

Have to do a bit of a dance to avoid hitting the interactive prompt
if there are no accounts configured.

* Remove unused methods

* Add some tests

* Fix linting errors

* Move fixtures to separate file

* Restructure mock test data and add more tests

* Add boilerplate

* Add test scenario for op v2 and increase coverage

* Fix up copyright statements

* Test v1 and v2 in all cases

* Use a more descriptive variable name

* Use docstrings rather than pass in abstract class

This adds coverage to abstract methods with the least amount of hackery.

* Increase test coverage for CLI classes

* Sort test parameters to avoid collection errors

* Update version tested in docs

* Revere test parameter sorting for now

The parameters need to be sorted to avoid the issue in older Python
versions in CI, but I’m having trouble working out how to do that
currently.

* Allow passing kwargs to the lookup module under test

* Favor label over id for v2 when looking for values

Add tests

* Display a warning for section on op v2 or greater

There is no “value” in section fields. If we wanted to support sections
in v2, we would also have to allow specifying the field name in
order to override “value”.

* Move test cases to their own file

Getting a bit unwieldy having it in the test file

* Move output into JSON files fore easier reuse

* Switch to using get_options()

* Add licenses for fixture files

* Use get_option() since get_options() was added in Ansible Core 2.12

* Rearrange fixtures

* Add changelog

* Move common classes to module_utils

* Move common classes back to lookup

The plugin relies on AnsibleLookupError() quite a bit which is not available
in module code.

Remove use of display for errors since section isn’t actually deprecated.

* Properly handle sections

Still room for improvement, but this is at least a start.

* Remove some comments that won’t be addressed

* Make test gathering more deterministic to avoid failures

* Update changelog fragment

* Simple fix for making tests reliable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

@ahussey-redhat ahussey-redhat ahussey-redhat left review comments

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug has_issue lookup lookup plugin module_utils module_utils new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

1Password lookup is broken with cli v2
6 participants
@samdoran @ansibullbot @codello @markuman @felixfontein @ahussey-redhat