-
Notifications
You must be signed in to change notification settings - Fork 1
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
replace lastpass cli with 1password cli #39
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
db_facts-3.8.4 | ||
db_facts-5.0.0 | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,20 +19,16 @@ def pull_lastpass_aws_iam(lastpass_entry_name: str) -> LastPassAWSIAM: | |
|
||
|
||
def lpass_field(name: str, field: str) -> str: | ||
if field == 'notes': | ||
field_arg = '--notes' | ||
elif field == 'username': | ||
field_arg = '--username' | ||
elif field == 'password': | ||
field_arg = '--password' | ||
elif field == 'url': | ||
field_arg = '--url' | ||
else: | ||
field_arg = '--field=' + field | ||
raw_output = check_output(['lpass', | ||
'show', | ||
field_arg, | ||
name]) | ||
# This used to use the lastpass-cli to pull credentials. But we've moved | ||
# from lastpass to 1password. This command retrieves the fields in the | ||
# same format from 1password instead. | ||
|
||
Comment on lines
+31
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make this comment even more explicit? I'd like it to make clear that ALL functions referencing LastPass/lpass are being redirected to pull data from 1Password instead. The names have not been changed in the interest of time. This needs to be an unmissable comment for anyone working on this in the future, so you can go wild with all caps. |
||
# Note this won't work for the way 1password stores notes and URLs, which | ||
# is different from lpass. But as of now db-facts doesn't ever rely on | ||
# these fields. | ||
raw_output = check_output( | ||
['op', 'item', 'get', name, '--field', f'label={field}']) | ||
|
||
return raw_output.decode('utf-8').rstrip('\n') | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,61 +4,47 @@ | |
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add an impossible-to-miss comment here with the same disclaimer from lpass.py |
||
class TestLPass(unittest.TestCase): | ||
@patch('db_facts.lpass.check_output') | ||
def test_lpass_field_notes(self, mock_check_output): | ||
mock_check_output.return_value = "fakenotes\n".encode("utf-8") | ||
out = lpass.lpass_field('my_name', 'notes') | ||
mock_check_output.\ | ||
assert_called_with(['lpass', 'show', '--notes', 'my_name']) | ||
assert out == "fakenotes" | ||
monikered marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
@patch('db_facts.lpass.check_output') | ||
def test_lpass_field_username(self, mock_check_output): | ||
mock_check_output.return_value = "fakeuser\n".encode("utf-8") | ||
out = lpass.lpass_field('my_name', 'username') | ||
mock_check_output.\ | ||
assert_called_with(['lpass', 'show', '--username', 'my_name']) | ||
mock_check_output.assert_called_with( | ||
['op', 'item', 'get', 'my_name', '--field', 'label=username']) | ||
assert out == "fakeuser" | ||
|
||
@patch('db_facts.lpass.check_output') | ||
def test_lpass_field_password(self, mock_check_output): | ||
mock_check_output.return_value = "fakepassword\n".encode("utf-8") | ||
out = lpass.lpass_field('my_name', 'password') | ||
mock_check_output.\ | ||
assert_called_with(['lpass', 'show', '--password', 'my_name']) | ||
mock_check_output.assert_called_with( | ||
['op', 'item', 'get', 'my_name', '--field', 'label=password']) | ||
assert out == "fakepassword" | ||
|
||
@patch('db_facts.lpass.check_output') | ||
def test_lpass_field_url(self, mock_check_output): | ||
mock_check_output.return_value = "fakeurl\n".encode("utf-8") | ||
out = lpass.lpass_field('my_name', 'url') | ||
mock_check_output.\ | ||
assert_called_with(['lpass', 'show', '--url', 'my_name']) | ||
assert out == "fakeurl" | ||
Comment on lines
-31
to
-37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same deal as with the Notes field test function -- I'd prefer for this to throw an error rather than be deleted entirely |
||
|
||
@patch('db_facts.lpass.check_output') | ||
def test_lpass_field_field1(self, mock_check_output): | ||
mock_check_output.return_value = "fakefield1\n".encode("utf-8") | ||
out = lpass.lpass_field('my_name', 'field1') | ||
mock_check_output.\ | ||
assert_called_with(['lpass', 'show', '--field=field1', 'my_name']) | ||
mock_check_output.assert_called_with( | ||
['op', 'item', 'get', 'my_name', '--field', 'label=field1']) | ||
assert out == "fakefield1" | ||
|
||
@patch('db_facts.lpass.check_output') | ||
def test_db_info_from_lpass(self, mock_check_output): | ||
def fake_check_output(args): | ||
assert args[0] == 'lpass' | ||
assert args[1] == 'show' | ||
assert args[0] == 'op' | ||
assert args[1] == 'item' | ||
assert args[2] == 'get' | ||
assert args[3] == 'my_lpass_name' | ||
assert args[4] == '--field' | ||
ret = { | ||
"--username": 'fakeuser', | ||
"--password": 'fakepassword', | ||
"--field=Hostname": 'fakehost', | ||
"--field=Port": '123', | ||
"--field=Type": 'faketype', | ||
"--field=Database": 'fakedatabase', | ||
"label=username": 'fakeuser', | ||
"label=password": 'fakepassword', | ||
"label=Hostname": 'fakehost', | ||
"label=Port": '123', | ||
"label=Type": 'faketype', | ||
"label=Database": 'fakedatabase', | ||
} | ||
return (ret[args[2]] + "\n").encode('utf-8') | ||
return (ret[args[5]] + "\n").encode('utf-8') | ||
mock_check_output.side_effect = fake_check_output | ||
db_info = lpass.db_info_from_lpass('my_lpass_name') | ||
expected_db_info = { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it normally refers to the Python version. Should that be updated here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Yes, this change should be reverted.
This refers to the python version selected for the local development python virtual environment. The venv gets created in deps.sh.
FWIW, circleci ignores this file so it doesn't affect any of those pipelines, only local dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh woops yeah i messed this file up mb