Skip to content

Authentication: Enable user impersonation for Superset to HiveServer2 using hive.server2.proxy.user (a.fernandez) #3652

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.

Sign up for GitHub

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

Jump to bottom
Merged
mistercrunch merged 1 commit into from
Oct 17, 2017
Merged

Authentication: Enable user impersonation for Superset to HiveServer2 using hive.server2.proxy.user (a.fernandez) #3652

mistercrunch merged 1 commit into from
Oct 17, 2017

Conversation

@afernandez
Copy link
Contributor

@afernandez afernandez commented Oct 12, 2017 •
edited
Loading

Superset today has a config for impersonation when creating/editing a datasource.
When used with Presto, it actually creates a connection on behalf of the logged on user.
For Hive, we instead want to connect as the superuser (superset service account) but use the hive.server2.proxy.user property in the URI to enable impersonation.

Unit Tests passed.

cc @timifasubaa @mistercrunch

@afernandez afernandez changed the title DI-1113. Authentication: Enable user impersonation for Superset to HiveServer2 using hive.server2.proxy.user (a.fernandez) Authentication: Enable user impersonation for Superset to HiveServer2 using hive.server2.proxy.user (a.fernandez) Oct 12, 2017
@afernandez
Copy link
Contributor Author

afernandez commented Oct 12, 2017

Need to fix unit tests

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
superset/db_engine_specs.py Outdated
Copy link
Contributor Author

@afernandez afernandez Oct 12, 2017

Choose a reason for hiding this comment

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

Base class has methods for how to modify a URI and URL object for impersonation

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
superset/db_engine_specs.py Outdated
Copy link
Contributor Author

@afernandez afernandez Oct 12, 2017

Choose a reason for hiding this comment

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

Python Engine Spec overrides the methods for how to modify a URI and URL object for impersonation

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
superset/models/core.py Outdated
Copy link
Contributor Author

@afernandez afernandez Oct 12, 2017

Choose a reason for hiding this comment

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

Moved this to its own method

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
superset/models/core.py Outdated
Copy link
Contributor Author

@afernandez afernandez Oct 12, 2017

Choose a reason for hiding this comment

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

Renamed uri to url since it was of type SQLAlchemy.URL

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
superset/models/core.py Outdated
Copy link
Contributor Author

@afernandez afernandez Oct 12, 2017

Choose a reason for hiding this comment

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

Important to mask the URL while logging since it may contain a password

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
superset/sql_lab.py Outdated
Copy link
Contributor Author

@afernandez afernandez Oct 12, 2017

Choose a reason for hiding this comment

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

In some code paths, conn was not defined

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
superset/views/core.py Outdated
Copy link
Contributor Author

@afernandez afernandez Oct 12, 2017

Choose a reason for hiding this comment

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

Important to mask the URL while logging since it may contain a password

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
tests/core_tests.py Outdated
Copy link
Contributor Author

@afernandez afernandez Oct 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Unit test is failing since g.user.username is missing. Will fix soon.

afernandez
afernandez commented Oct 12, 2017
View reviewed changes
Copy link
Contributor Author

@afernandez afernandez left a comment

Choose a reason for hiding this comment

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

Initial annotations

@afernandez afernandez force-pushed the afernandez_impersonate branch from eee12f1 to 4e9263e Compare October 12, 2017 23:39
@afernandez
Copy link
Contributor Author

afernandez commented Oct 12, 2017

Unit tests passed,

Ran 168 tests in 268.075s

OK
Submitting coverage to coveralls.io...
Coverage submitted!
Job ##8777.1
https://coveralls.io/jobs/30207773

@coveralls
Copy link

coveralls commented Oct 12, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.08%) to 70.191% when pulling 4e9263e804d4297a94edf81aaeaf1b1f39cfd04d on afernandez:afernandez_impersonate into 52a9f27 on apache:master.

mistercrunch
mistercrunch requested changes Oct 13, 2017
View reviewed changes
Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Highlighted a few minor things but LGTM otherwise. Please lint according to our .pylinrc. We used to have automation with Landscape.io but switched it off since we move the repo to Apache...

superset/db_engine_specs.py Outdated
Copy link
Member

@mistercrunch mistercrunch Oct 13, 2017

Choose a reason for hiding this comment

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

NIT: I'd go without the is not None as None evals to False

Copy link
Contributor Author

@afernandez afernandez Oct 13, 2017

Choose a reason for hiding this comment

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

Fixed

superset/db_engine_specs.py Outdated
Copy link
Member

@mistercrunch mistercrunch Oct 13, 2017

Choose a reason for hiding this comment

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

Some very long lines here. PEP8 says 80, our pylint say 90. If you want to lint your PR only you can git diff master... | flake8 --diff thought that's flake8 not pylint. There's also git-lint which can lint your diff as well.

Copy link
Contributor Author

@afernandez afernandez Oct 13, 2017

Choose a reason for hiding this comment

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

Fixed

superset/db_engine_specs.py Outdated
Copy link
Member

@mistercrunch mistercrunch Oct 13, 2017

Choose a reason for hiding this comment

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

NIT: -is not None

Copy link
Contributor Author

@afernandez afernandez Oct 13, 2017

Choose a reason for hiding this comment

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

Fixed

DI-1113. Authentication: Enable user impersonation for Superset to Hi…
f310817 
…veServer2 using hive.server2.proxy.user (a.fernandez)
@afernandez afernandez force-pushed the afernandez_impersonate branch from 4e9263e to f310817 Compare October 13, 2017 23:03
@coveralls
Copy link

coveralls commented Oct 13, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.02%) to 70.364% when pulling f310817 on afernandez:afernandez_impersonate into bad6938 on apache:master.

@afernandez
Copy link
Contributor Author

afernandez commented Oct 16, 2017

@mistercrunch would you be able to re-review these changes? Does the decrease in coveralls mean I need to add unit tests?

@mistercrunch mistercrunch merged commit adef519 into apache:master Oct 17, 2017
@afernandez
Copy link
Contributor Author

afernandez commented Oct 20, 2017

Thanks for reviewing. I figured out how to do it without needing changes to PyHive. Will submit another PR soon.
@mistercrunch

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
@afernandez @michellethomas
DI-1113. Authentication: Enable user impersonation for Superset to Hi…
dd1454d 
…veServer2 using hive.server2.proxy.user (a.fernandez) (apache#3652)
@snyk-bot snyk-bot mentioned this pull request Aug 19, 2021
Closed
@m-ajay m-ajay mentioned this pull request Sep 27, 2022
Open
This was referenced Oct 6, 2022
Open
Open
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.5 First shipped in 0.20.5 labels Feb 27, 2024
@m-ajay m-ajay mentioned this pull request Mar 15, 2024
Open
@m-ajay m-ajay mentioned this pull request Mar 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mistercrunch mistercrunch mistercrunch approved these changes

Assignees

No one assigned

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.5 First shipped in 0.20.5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@afernandez @coveralls @mistercrunch