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

Fix user impersonation with presto #9422

Closed
wants to merge 1 commit into from

Conversation

tooptoop4
Copy link
Contributor

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Fixes #9406
PyHive 0.6.2 release includes dropbox/PyHive#309 making this change possible.
In presto, credentials are authenticated against a 'principal', that principal can then impersonate a 'user' so queries can be executed with authorisation rules applied to the user (not the principal).
Benefit: a single shared presto data source can be created in superset with a generic account (superuser aka the principal) in the presto connection. Then each user that logs into superset ui does not need to create new datasource, they can go straight to SQLLab and run presto queries on the shared datasource which are being authorised under their username.

@etr2460
Copy link
Member

etr2460 commented Mar 31, 2020

@john-bodley this might be interesting to you

Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

Looking at dropbox/PyHive#309 I'm unsure that setting principle_username provides any additional value over simply setting the url.username per the Presto logic here if user impersonation is enabled.

self.db_engine_spec.modify_url_for_impersonation(
sqlalchemy_url, self.impersonate_user, effective_username
)
if not user_name or not str(sqlalchemy_url).startswith('presto'):
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be any database specific logic in core.py hence the reason for the self.db_engine_spec.modify_url_for_impersonation(...) method. You should do something like this.

@@ -313,6 +314,9 @@ def get_sqla_engine(
if connect_args:
params["connect_args"] = connect_args

if user_name and self.impersonate_user and str(sqlalchemy_url).startswith('presto'):
Copy link
Member

Choose a reason for hiding this comment

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

See ^^. You should update get_configuration_for_impersonation to handle this. Note given that this isn't part of the connection key the get_configuration_for_impersonation needs be refactored to be get_connection_args_for_impersonation.

@tooptoop4
Copy link
Contributor Author

@john-bodley current behavior before this pr seems to modify username in url so it expects all users to have same password as the generic acct !?

@john-bodley
Copy link
Member

@tooptoop4 you could do something similar to #9405 to achieve what you require which also updates connect_args.

@stale
Copy link

stale bot commented May 30, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label May 30, 2020
@tooptoop4
Copy link
Contributor Author

crispy

@stale stale bot removed the inactive Inactive for >= 30 days label May 30, 2020
@stale
Copy link

stale bot commented Jul 30, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jul 30, 2020
@stale stale bot closed this Aug 8, 2020
@DRavikanth
Copy link

@tooptoop4 curious to know why this PR was ever merged to superset branch?

@tooptoop4
Copy link
Contributor Author

@DRavikanth I'm not a committer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Password not replaced in SQLAlchemy URL when impersonation is enabled causing 401 Unauthorized
4 participants