You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/27 12:06:41 UTC
[GitHub] [incubator-superset] tjmateus opened a new pull request #9405: Added support for impersonation on SQL Server databases using the pym…
tjmateus opened a new pull request #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405
…ssql adapter.
### CATEGORY
Choose one
- [ ] Bug Fix
- [ x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
By default, the impersonation on Superset for SQL Server connections is done by replacing the user of the connection string by the logged in user name. This was not the behaviour we were expecting at DefinedCrowd. We want to have an application user capable of impersonating the logged in user.
The solution I'm proposing is to use the mssql+pymssql SQLAlchemy adapter you already support and use the conn_properties query parameter, which allows us to send SQL queries to the server upon the connection establishment. This query would be EXECUTE AS USER = '<USER_TO_IMPERSONATE>', which will run all the following queries as the logged in user.
### TEST PLAN
####Exploratory test
1. Create a database user for your logged in user. They **must** share the same username;
2. Create an application user on a SQL Server database. This user must be granted
with the `IMPERSONATE` permission as below:
```
GRANT IMPERSONATE ON USER::[<USER_TO_IMPERSONATE>] TO <APPLICATION_USER>;
```
3. On Superset UI, create a new database source with a connection with the following syntax:
```
mssql+pymssql://<username>:<password>@<server>:<port>/<database_name>
```
4. Enable the `Impersonate the logged on user` flag;
5. Run the query `SELECT CURRENT_USER as username;` on SQL Lab;
6. The expected output should be your logged in user and not the application one.
####Unit test
If you think this feature would be useful for community, I will invest time on this kind of tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405#discussion_r399389971
##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -76,3 +77,16 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
if regex.match(type_):
return sqla_type
return None
+
+ @classmethod
+ def modify_url_for_impersonation(
+ cls, url: URL, impersonate_user: bool, username: Optional[str]
+ ) -> None:
+ """
+ Modify the SQL Alchemy URL object with the user to impersonate if applicable.
+ :param url: SQLAlchemy URL object
+ :param impersonate_user: Flag indicating if impersonation is enabled
+ :param username: Effective username
+ """
+ if impersonate_user and username is not None:
+ url.query["conn_properties"] = "EXECUTE AS USER = '{}'".format(username)
Review comment:
We tend to use f-strings, e.g. `abc = f"variable goes {here}"`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405#issuecomment-609962980
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9405?src=pr&el=h1) Report
> Merging [#9405](https://codecov.io/gh/apache/incubator-superset/pull/9405?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/81971967c3df4d430c08b52dfce7685d7fd44264&el=desc) will **decrease** coverage by `0.00%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9405?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9405 +/- ##
==========================================
- Coverage 59.01% 59.01% -0.01%
==========================================
Files 377 383 +6
Lines 12162 12191 +29
Branches 3004 3014 +10
==========================================
+ Hits 7177 7194 +17
- Misses 4805 4813 +8
- Partials 180 184 +4
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9405?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/explore/controlUtils.js](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFV0aWxzLmpz) | `89.36% <0.00%> (-0.86%)` | :arrow_down: |
| [superset-frontend/src/preamble.js](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ByZWFtYmxlLmpz) | `0.00% <0.00%> (ø)` | |
| [superset-frontend/src/chart/chartReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L2NoYXJ0UmVkdWNlci5qcw==) | `19.04% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `35.39% <0.00%> (ø)` | |
| [...et-frontend/src/explore/controlPanels/sections.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9zZWN0aW9ucy5qc3g=) | `100.00% <0.00%> (ø)` | |
| [...et-frontend/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvZXhwbG9yZVJlZHVjZXIuanM=) | `25.00% <0.00%> (ø)` | |
| [...-frontend/src/explore/controlPanels/DeckScatter.js](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9EZWNrU2NhdHRlci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...-frontend/src/explore/reducers/saveModalReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvc2F2ZU1vZGFsUmVkdWNlci5qcw==) | `0.00% <0.00%> (ø)` | |
| [...rontend/src/visualizations/FilterBox/FilterBox.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL0ZpbHRlckJveC9GaWx0ZXJCb3guanN4) | `5.08% <0.00%> (ø)` | |
| [...nd/src/explore/components/ExploreViewContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FeHBsb3JlVmlld0NvbnRhaW5lci5qc3g=) | `46.47% <0.00%> (ø)` | |
| ... and [15 more](https://codecov.io/gh/apache/incubator-superset/pull/9405/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9405?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9405?src=pr&el=footer). Last update [8197196...8fea0b0](https://codecov.io/gh/apache/incubator-superset/pull/9405?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] tjmateus commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
tjmateus commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405#discussion_r403982261
##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -76,3 +77,16 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
if regex.match(type_):
return sqla_type
return None
+
+ @classmethod
+ def modify_url_for_impersonation(
+ cls, url: URL, impersonate_user: bool, username: Optional[str]
+ ) -> None:
+ """
+ Modify the SQL Alchemy URL object with the user to impersonate if applicable.
+ :param url: SQLAlchemy URL object
+ :param impersonate_user: Flag indicating if impersonation is enabled
+ :param username: Effective username
+ """
+ if impersonate_user and username is not None:
+ url.query["conn_properties"] = "EXECUTE AS USER = '{}'".format(username)
Review comment:
Hi @villebro, did you have a chance to think on my question? Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405#issuecomment-609715907
@tjmateus one more thing, could you add a note under `docs/installation.rst` and add instructions for how this works with `pymssql`? This make it more accessible to other users, too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405#discussion_r403986473
##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -76,3 +78,19 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
if regex.match(type_):
return sqla_type
return None
+
+ @classmethod
+ def modify_url_for_impersonation(
+ cls, url: URL, impersonate_user: bool, username: Optional[str]
+ ) -> None:
+ """
+ Modify the SQL Alchemy URL object with the user to impersonate if applicable.
+ :param url: SQLAlchemy URL object
+ :param impersonate_user: Flag indicating if impersonation is enabled
+ :param username: Effective username
+ """
+ if impersonate_user and username is not None:
+ if url.get_dialect().driver == MSDialect_pymssql.driver:
+ url.query["conn_properties"] = f"EXECUTE AS USER = '{username}'"
+ else:
+ url.username = username
Review comment:
I would turn this around, ie. check if the driver is `MSDialect_pymssql`, then do the different logic, else call the original method `super(). modify_url_for_impersonation(url, impersonate_user, username)`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] tjmateus commented on issue #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
tjmateus commented on issue #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405#issuecomment-610030579
> Looking good @tjmateus 👍 Will leave this here for a while for others to chime in, but this looks ready to me.
Thanks for all your inputs, @villebro. They were really useful!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] tjmateus commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
tjmateus commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405#discussion_r403991259
##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -76,3 +78,19 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
if regex.match(type_):
return sqla_type
return None
+
+ @classmethod
+ def modify_url_for_impersonation(
+ cls, url: URL, impersonate_user: bool, username: Optional[str]
+ ) -> None:
+ """
+ Modify the SQL Alchemy URL object with the user to impersonate if applicable.
+ :param url: SQLAlchemy URL object
+ :param impersonate_user: Flag indicating if impersonation is enabled
+ :param username: Effective username
+ """
+ if impersonate_user and username is not None:
+ if url.get_dialect().driver == MSDialect_pymssql.driver:
+ url.query["conn_properties"] = f"EXECUTE AS USER = '{username}'"
+ else:
+ url.username = username
Review comment:
Makes sense! Already made the requested changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] tjmateus commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
tjmateus commented on a change in pull request #9405: Added support for impersonation on SQL Server databases using the pym…
URL: https://github.com/apache/incubator-superset/pull/9405#discussion_r399421308
##########
File path: superset/db_engine_specs/mssql.py
##########
@@ -76,3 +77,16 @@ def get_sqla_column_type(cls, type_: str) -> Optional[TypeEngine]:
if regex.match(type_):
return sqla_type
return None
+
+ @classmethod
+ def modify_url_for_impersonation(
+ cls, url: URL, impersonate_user: bool, username: Optional[str]
+ ) -> None:
+ """
+ Modify the SQL Alchemy URL object with the user to impersonate if applicable.
+ :param url: SQLAlchemy URL object
+ :param impersonate_user: Flag indicating if impersonation is enabled
+ :param username: Effective username
+ """
+ if impersonate_user and username is not None:
+ url.query["conn_properties"] = "EXECUTE AS USER = '{}'".format(username)
Review comment:
You are right, `pymssql` is deprecated and this feature is limited to that adapter. I've analyzed first `pyodbc` but I didn't find a way to send SQL queries to the server upon the connection establishment. I added logic to check the driver being used and applied this logic only for `pymssql`. For other adapters I just don't know if we should raise an exception or just add the default behaviour, which is replacing the connection string username by the logged in user's. What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stale[bot] commented on pull request #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #9405:
URL: https://github.com/apache/incubator-superset/pull/9405#issuecomment-657061724
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] stale[bot] closed pull request #9405: Added support for impersonation on SQL Server databases using the pym…
Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #9405:
URL: https://github.com/apache/incubator-superset/pull/9405
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org