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.

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

Feature: "Impersonate user" setting on Datasource #3404

Merged
merged 5 commits into from
Sep 18, 2017
Merged

Feature: "Impersonate user" setting on Datasource #3404

merged 5 commits into from
Sep 18, 2017

Conversation

dmigo
Copy link
Contributor

@dmigo dmigo commented Sep 1, 2017 •
edited
Loading

Implements #2148

  • New setting for Datasources called "Impersonate user"
  • Depending on the setting username is going to be included in the connection string for a sync execution
  • Depending on the setting username is going to be included in the connection string for an async execution

All the code that makes difference is covered, I don't really think it would make sense to add any more tests as a part of the current PR.

@dmigo
Copy link
Contributor Author

dmigo commented Sep 1, 2017

Current solution works for sync queries but doesn't for async. It says

'_AppCtxGlobals' object has no attribute 'user'

Which makes sense. My first idea would be to pass it as a parameter to get_sql_results for the async case.

@dmigo dmigo force-pushed the master branch 2 times, most recently from 659c87d to ddd7c66 Compare September 1, 2017 16:16
@coveralls
Copy link

coveralls commented Sep 1, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 69.137% when pulling ddd7c66e200c6f666fa7ce0d7a1a0493e6b8ff92 on dmigo:master into 3dfdde1 on apache:master.

@coveralls
Copy link

coveralls commented Sep 1, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 69.137% when pulling ddd7c66e200c6f666fa7ce0d7a1a0493e6b8ff92 on dmigo:master into 3dfdde1 on apache:master.

@coveralls
Copy link

coveralls commented Sep 1, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 69.137% when pulling 018017d11bf9ee0ccf53aa0f71643493ee86445c on dmigo:master into 3dfdde1 on apache:master.

@dmigo dmigo changed the title [WIP] Add "Impersonate user" setting to Datasource Add "Impersonate user" setting to Datasource Sep 3, 2017
@coveralls
Copy link

coveralls commented Sep 3, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 69.137% when pulling fa9b606e9cb7edac1c35d30af34c46e70b57d9d5 on dmigo:master into 3dfdde1 on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 69.137% when pulling fa9b606e9cb7edac1c35d30af34c46e70b57d9d5 on dmigo:master into 3dfdde1 on apache:master.

@dmigo dmigo changed the title Add "Impersonate user" setting to Datasource [wip] Add "Impersonate user" setting to Datasource Sep 5, 2017
@dmigo dmigo changed the title [wip] Add "Impersonate user" setting to Datasource Add "Impersonate user" setting to Datasource Sep 7, 2017
@coveralls
Copy link

coveralls commented Sep 7, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.001%) to 69.117% when pulling 0945f55877fa883e366fbfa7005af9b179aa527a on dmigo:master into 3dfdde1 on apache:master.

@coveralls
Copy link

coveralls commented Sep 12, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.001%) to 69.117% when pulling a62f02e3cb3474a43122ec09fabd4e02773a17cc on dmigo:master into 147c12d on apache:master.

@coveralls
Copy link

coveralls commented Sep 12, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.001%) to 69.111% when pulling b92e03d155397bdf406e2e7f564c47c7ef074b80 on dmigo:master into 7c1b56f on apache:master.

@dmigo dmigo changed the title Add "Impersonate user" setting to Datasource Feature: "Impersonate user" setting to Datasource Sep 12, 2017
@dmigo dmigo changed the title Feature: "Impersonate user" setting to Datasource Feature: "Impersonate user" setting on Datasource Sep 12, 2017
@coveralls
Copy link

coveralls commented Sep 12, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.001%) to 69.118% when pulling 95f1b0d7c5e8ee12bb7c06ad4e01e380b2301b5a on dmigo:master into 3c0e85e on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 69.118% when pulling 95f1b0d7c5e8ee12bb7c06ad4e01e380b2301b5a on dmigo:master into 3c0e85e on apache:master.

@coveralls
Copy link

coveralls commented Sep 14, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.001%) to 69.153% when pulling 31e78051a88597ce651e7de2332de138980b1d07 on dmigo:master into fdee06b on apache:master.

mistercrunch
mistercrunch requested changes Sep 15, 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.

Minor change request, otherwise looks good.

Took me a moment to understand why you're passing the username around, and then I understood that in the async context the user isn't authenticated.

Good work!

superset/models/core.py Outdated
extra = self.get_extra()
uri = make_url(self.sqlalchemy_uri_decrypted)
params = extra.get('engine_params', {})
if nullpool:
params['poolclass'] = NullPool
uri = self.db_engine_spec.adjust_database_uri(uri, schema)
if self.impersonate_user:
uri.username = user_name if user_name != None else g.user.username
Copy link
Member

Choose a reason for hiding this comment

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

I prefer user_name if user_name else g.user.username as it accounts for empty strings and it's more pythonesque

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! 👍

@coveralls
Copy link

coveralls commented Sep 17, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.001%) to 69.14% when pulling 5c02f71 on dmigo:master into e22aecb on apache:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 69.14% when pulling 5c02f71 on dmigo:master into e22aecb on apache:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 69.14% when pulling 5c02f71 on dmigo:master into e22aecb on apache:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 69.14% when pulling 5c02f71 on dmigo:master into e22aecb on apache:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 69.14% when pulling 5c02f71 on dmigo:master into e22aecb on apache:master.

@mistercrunch mistercrunch merged commit c988080 into apache:master Sep 18, 2017
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request Oct 3, 2017
@dmigo @timifasubaa
Feature: "Impersonate user" setting on Datasource (apache#3404)
54cf51a 
* Add "Impersonate user" setting to Datasource

* Add tests

* Use g.user.username for all the sync cases

* use uri.username instead of uri.user

* Small refactoring
@john-bodley john-bodley mentioned this pull request Mar 5, 2018
Merged
@mjgp2
Copy link

mjgp2 commented Mar 6, 2018

Is there anything in the docs about this?

Seems like it sets the DB user in the connection string to the superset user name, but I don't understand how the password for that DB user is set?

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
@dmigo @michellethomas
Feature: "Impersonate user" setting on Datasource (apache#3404)
f5855db 
* Add "Impersonate user" setting to Datasource

* Add tests

* Use g.user.username for all the sync cases

* use uri.username instead of uri.user

* Small refactoring
@tooptoop4 tooptoop4 mentioned this pull request Mar 30, 2020
Merged
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistercrunch mistercrunch mistercrunch requested 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.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmigo @coveralls @mjgp2 @mistercrunch