Skip to content

Commit

Permalink
Fix broken password property and flag (#58)
Browse files Browse the repository at this point in the history
In previous releases the password set via the config file or command
line was ignored. If the keyring was enabled it was used, if it was
not set then user was prompted for the password.

Now the password property and flag are respected. If the keyring
is enabled it is used, and the password property and flag are still
ignored. If the keyring is enabled, but not yet set then the user
is prompted even if a password is given via the password property
or flag. The keyring is preferred because it is more secure storage.
  • Loading branch information
ddriddle authored Feb 3, 2021
1 parent 03b5d01 commit 0f21132
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 14 deletions.
28 changes: 17 additions & 11 deletions src/awscli_login/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,21 +279,27 @@ def get_username(self):
username = getuser()
self.username = input("Username [%s]: " % username) or username

def get_password(self):
""" Get password from user if necessary. """
if self.enable_keyring:
if self.password is not None:
logger.warn('Using keyring: Ignoring password set via'
' configuration file or command line.')

ukey = self.username + '@' + urlparse(self.ecp_endpoint_url).netloc
self.password = get_password("awscli_login", ukey)

if self.password is None:
self.password = getpass()

if self.enable_keyring:
set_password("awscli_login", ukey, self.password)

# Can we make this DRYer?
def get_credentials(self, first_pass: bool = True) -> Creds:
""" Get credentials from user if necessary. """
self.get_username()

password = None
if self.enable_keyring:
ukey = self.username + '@' + urlparse(self.ecp_endpoint_url).netloc
password = get_password("awscli_login", ukey)
if password is None:
password = getpass()
if self.enable_keyring:
set_password("awscli_login", ukey, password)
assert isinstance(password, str), "Password is not a string!"
self.password = password
self.get_password()

# TODO add support for any factor...
# https://github.com/JohnPfeifer/duo-non-browser/wiki
Expand Down
97 changes: 94 additions & 3 deletions src/tests/config/test_get_credentials.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ def mock_get_credentials_inputs(self, inputs: Creds):
mock_password = self.patcher(
'awscli_login.config.getpass',
autospec=True,
side_effect=[inputs.password]
# getpass should always return a string value!
side_effect=[inputs.password if inputs.password else '']
)

mock_get_password = self.patcher(
Expand Down Expand Up @@ -119,13 +120,18 @@ def assertGetCredentialsMocksCalled(
)

def _test_get_credentials(self, outputs: Creds) -> None:
usr, pwd, hdr = self.profile.get_credentials()

self._test_get_credentials_outputs(usr, pwd, hdr, outputs)

def _test_get_credentials_outputs(self, usr, pwd, hdr,
outputs: Creds) -> None:

# Ensure actual outputs equal expected outputs
errors = []
mesg = 'get_credentials returned %s: %s. Expected: %s.'
self.outputs = outputs

usr, pwd, hdr = self.profile.get_credentials()

pairs = [
('username', usr),
('password', pwd),
Expand Down Expand Up @@ -287,3 +293,88 @@ def test_get_credentials_keyring_test_no_prompt(self):
mocks = self.mock_get_credentials_inputs(inputs)
self._test_get_credentials(outputs)
self.assertGetCredentialsMocksCalled(*mocks)


class GetCredsWithArgsMinProfileTest(GetCredsProfileBase):
""" Test that cli arguments are able to override profile settings
and prevent the user from being prompted for the value set. """

def _test_clioverrides_profile(self, inputs: Creds, outputs: Creds,
**kwargs):
""" Test that get_credentials_inputs returns given outputs
when supplied with mocked user inputs.
inputs: Values used to mock user input.
outputs: Expected values set on profile after get_credentials is
called with mocked inputs.
kwargs: Command line arguments passed to user profile.
"""
# Pass cli arguments
self.Profile(**kwargs)

# Call get_credentials with mocked input
mocks = self.mock_get_credentials_inputs(inputs)
usr, pwd, hdr = self.profile.get_credentials()

# Ensure that only expected values asked for
self.assertGetCredentialsMocksCalled(*mocks)

# Ensure that values returned match expected values
self._test_get_credentials_outputs(usr, pwd, hdr, outputs)

def test_get_credentials_password_arg_set(self):
""" Prompt for just username when password cli flag is set. """
self.login_config = """
[default]
ecp_endpoint_url = foo
factor = off
"""
pwd = "1234"
inputs = Creds(username="user")
outputs = Creds(username="user", password=pwd)

self._test_clioverrides_profile(inputs, outputs, password=pwd)

def test_get_credentials_username_password_set_in_config(self):
""" Prompt for nothing when username & password properties are set. """
self.login_config = """
[default]
ecp_endpoint_url = foo
factor = off
password = 1234
username = foo
"""
inputs = Creds()
outputs = Creds(username="foo", password="1234")

self._test_clioverrides_profile(inputs, outputs)

def test_get_credentials_use_keyring_instead_of_property(self):
""" Use keyring even if password property is set. """
self.login_config = """
[default]
ecp_endpoint_url = foo
enable_keyring = True
factor = off
password = 1234
username = foo
"""
inputs = Creds(keyring="secret")
outputs = Creds(username="foo", password="secret")

self._test_clioverrides_profile(inputs, outputs)

def test_get_credentials_use_keyring_instead_of_flag(self):
""" Use keyring even if password flag is set. """
self.login_config = """
[default]
ecp_endpoint_url = foo
enable_keyring = True
factor = off
username = foo
"""
inputs = Creds(keyring="secret")
outputs = Creds(username="foo", password="secret")

self._test_clioverrides_profile(inputs, outputs)

0 comments on commit 0f21132

Please sign in to comment.