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

Passing username for impersonation #334

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gbrian
Copy link

@gbrian gbrian commented Feb 4, 2019

No description provided.

@timarmstrong
Copy link
Contributor

Can you add a test for this feature? Maybe you can use the effective_user() builtin function to make sure that it had the intended effect.

@gbrian
Copy link
Author

gbrian commented Feb 27, 2019

Do you mean kind of:

def test_sqlalchemy_impersonation():
    engine = create_engine('impala://localhost')
    result = connection.execute("select effective_user() as effective_user")
    observed = result[0]['effective_user']
    expected = '?????'
    assert expected == observed

    impersonate = 'user1'
    engine = create_engine('impala://%s@localhost' % impersonate)
    result = connection.execute("select effective_user() as effective_user")
    observed = result[0]['effective_user']
    expected = impersonate
    assert expected == observed

@timarmstrong
Copy link
Contributor

Something along those lines, yeah. I think the second part of that test is the most important. Mainly to prove that the 'effective_user' value gets plumbed all the way through to Impala correctly.

Copy link
Contributor

@timarmstrong timarmstrong left a comment

Choose a reason for hiding this comment

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

Can you add a test for this potentially?

@@ -41,7 +41,7 @@ def connect(host='localhost', port=21050, database=None, timeout=None,
use_ssl=False, ca_cert=None, auth_mechanism='NOSASL', user=None,
password=None, kerberos_service_name='impala', use_ldap=None,
ldap_user=None, ldap_password=None, use_kerberos=None,
protocol=None):
protocol=None,username=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after comma to be consistent with surrounding code and PEP8

log.debug('HiveServer2Connection(service=%s, default_db=%s)', service,
default_db)
self.service = service
self.default_db = default_db
self.impersonate=impersonate
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spaces around = to be consistent with surrounding code and PEP8

@@ -122,6 +124,12 @@ def cursor(self, user=None, configuration=None, convert_types=True,

log.debug('.cursor(): getting new session_handle')

if self.impersonate != None:
Copy link
Contributor

Choose a reason for hiding this comment

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

@huw0
Copy link
Contributor

huw0 commented May 22, 2023

#281 indicates that impersonation works on a per-cursor basis.

Therefore can this PR be closed? I think it will be confusing to support impersonation in multiple places and this could lead to unintentional security issues where connections are re-used.

@gbrian gbrian closed this May 23, 2023
@gbrian
Copy link
Author

gbrian commented May 23, 2023

Not needed

@gbrian gbrian reopened this May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants