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

Explicit profile overrides environment variables #486

Merged
merged 7 commits into from
Mar 18, 2015
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions botocore/credentials.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ def create_credential_resolver(session):
config_file = session.get_config_variable('config_file')
metadata_timeout = session.get_config_variable('metadata_service_timeout')
num_attempts = session.get_config_variable('metadata_service_num_attempts')

providers = [
EnvProvider(),
SharedCredentialProvider(
creds_filename=credential_file,
profile_name=profile_name
Expand All @@ -63,6 +63,18 @@ def create_credential_resolver(session):
num_attempts=num_attempts)
)
]

# We use ``session.profile`` for EnvProvider rather than
# ``profile_name`` because it is ``None`` when unset.
if session.profile is None:
# No profile has been explicitly set, so we prepend the environment
# variable provider. That provider, in turn, may set a profile
# or credentials.
providers.insert(0, EnvProvider())
else:
logger.info('Skipping environment variable credential check'
Copy link
Member

Choose a reason for hiding this comment

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

This seems more like a debug message. We'll still log which provider ultimately gives creds, but the fact that we're disabling a check seems mostly of interest to a dev.

' because profile name was explicitly set.')

resolver = CredentialResolver(providers=providers)
return resolver

Expand Down Expand Up @@ -580,7 +592,7 @@ def load_credentials(self):
# If we got here, no credentials could be found.
# This feels like it should be an exception, but historically, ``None``
# is returned.
#
#
# +1
# -js
return None
7 changes: 7 additions & 0 deletions tests/integration/test-credentials
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[default]
aws_access_key_id = default
aws_secret_access_key = default-secret

[test]
aws_access_key_id = test
aws_secret_access_key = test-secret
106 changes: 106 additions & 0 deletions tests/integration/test_credentials.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# Copyright 2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"). You
# may not use this file except in compliance with the License. A copy of
# the License is located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
# ANY KIND, either express or implied. See the License for the specific
# language governing permissions and limitations under the License.

import os
import mock

from botocore.session import Session
from tests import unittest


class TestCredentialPrecedence(unittest.TestCase):
def setUp(self):
# Clean up any existing environment
for name in ['AWS_ACCESS_KEY_ID',
'AWS_SECRET_ACCESS_KEY',
'BOTO_DEFAULT_PROFILE']:
if name in os.environ:
del os.environ[name]
Copy link
Member

Choose a reason for hiding this comment

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

This is general test case best practice advice: Do not leave global state modified outside the scope of a test. It's ok to mess with global state in a test if there's no better way to test what you need to test, but you have to put the state back to the way it initially was.

Here, if the vars you're deleting were defined in os.environ before the test, this setUp() is deleting these values and not restoring them after the test executes. This can lead to all sorts of hard to track down problems where the test order may affect whether or not a test passes depending on if it passes before/after the global state modification.

That being, said, just use BaseEnvVar to handle the patching of env vars for you.


# Set the config file to something that doesn't exist, then
# set the shared credential file to our test config.
os.environ['AWS_CONFIG_FILE'] = '~/.aws/config-missing'
Session.SessionVariables['credentials_file'] = (
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here. You are modifying a class variable, which persists across tests. Avoid doing this, as this is modifying global state and not putting it back.

None, None,
os.path.join(os.path.dirname(__file__), 'test-credentials'))

def test_access_secret_vs_profile_env(self):
# If all three are given, then the access/secret keys should
# take precedence.
os.environ['AWS_ACCESS_KEY_ID'] = 'env'
os.environ['AWS_SECRET_ACCESS_KEY'] = 'env-secret'
os.environ['BOTO_DEFAULT_PROFILE'] = 'test'

s = Session()
credentials = s.get_credentials()

self.assertEqual(credentials.access_key, 'env')
self.assertEqual(credentials.secret_key, 'env-secret')

@mock.patch('botocore.credentials.Credentials')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really a fan of patching, especially when it's not necessary. Why can't we just check the credential object directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately this isn't exposed anywhere in clients. There is no way to get the credentials using public interfaces once a client is created. I'm not opposed to adding a meta.credentials property or something on clients, but I figured there would be push-back if I add this just for a test.

def test_access_secret_vs_profile_code(self, credentials_cls):
# If all three are given, then the access/secret keys should
# take precedence.
s = Session()
s.profile = 'test'

client = s.create_client('s3', aws_access_key_id='code',
aws_secret_access_key='code-secret')

credentials_cls.assert_called_with(
access_key='code', secret_key='code-secret', token=mock.ANY)

def test_profile_env_vs_code(self):
# If the profile is set both by the env var and by code,
# then the one set by code should take precedence.
os.environ['BOTO_DEFAULT_PROFILE'] = 'test'
s = Session()
s.profile = 'default'

credentials = s.get_credentials()

self.assertEqual(credentials.access_key, 'default')
self.assertEqual(credentials.secret_key, 'default-secret')

@mock.patch('botocore.credentials.Credentials')
def test_access_secret_env_vs_code(self, credentials_cls):
# If the access/secret keys are set both as env vars and via
# code, then those set by code should take precedence.
os.environ['AWS_ACCESS_KEY_ID'] = 'env'
os.environ['AWS_SECRET_ACCESS_KEY'] = 'secret'
s = Session()

client = s.create_client('s3', aws_access_key_id='code',
aws_secret_access_key='code-secret')

credentials_cls.assert_called_with(
access_key='code', secret_key='code-secret', token=mock.ANY)

@mock.patch('botocore.credentials.Credentials')
def test_access_secret_env_vs_profile_code(self, credentials_cls):
# If access/secret keys are set in the environment, but then a
# specific profile is passed via code, then the access/secret
# keys defined in that profile should take precedence over
# the environment variables. Example:
#
# ``aws --profile dev s3 ls``
#
os.environ['AWS_ACCESS_KEY_ID'] = 'env'
os.environ['AWS_SECRET_ACCESS_KEY'] = 'env-secret'
s = Session()
s.profile = 'test'

client = s.create_client('s3')

credentials_cls.assert_called_with(
'test', 'test-secret', None, method='shared-credentials-file')
1 change: 0 additions & 1 deletion tests/unit/test_credentials.py
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,6 @@ def test_partial_creds_is_an_error(self):
with self.assertRaises(botocore.exceptions.PartialCredentialsError):
provider.load()


class TestSharedCredentialsProvider(BaseEnvVar):
def setUp(self):
super(TestSharedCredentialsProvider, self).setUp()
Expand Down