You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org> on 2023/02/16 18:47:24 UTC
[GitHub] [superset] Antonio-RiveroMartnez opened a new pull request, #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Antonio-RiveroMartnez opened a new pull request, #23099:
URL: https://github.com/apache/superset/pull/23099
### SUMMARY
We are now enabling users to export databases that have SSHTunnels related to them. The exported ssh_tunnel will get included in the `database` `yaml` file and will have the used login method (`password` or `private_key + private_key_password`) with masked values.
Then when user tries to import such databases (or any dashboard/chart/dataset/savedquery), the modal will ask the user to enter such credentials.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
1. Export a DB with SSH Tunnel
2. The exported file should include the `ssh_tunnel` with masked login method
3. Try importing a DB that has `ssh_tunnel` in it
4. The modal should ask you to enter the credentials (`password` OR `private_key + private_key_password`)
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [X] Introduces new feature or API
- [ ] Removes existing feature or API
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] Antonio-RiveroMartnez closed pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez closed pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
URL: https://github.com/apache/superset/pull/23099
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] hughhhh commented on a diff in pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #23099:
URL: https://github.com/apache/superset/pull/23099#discussion_r1110219873
##########
superset/databases/schemas.py:
##########
@@ -719,6 +723,65 @@ def validate_password(self, data: Dict[str, Any], **kwargs: Any) -> None:
if password == PASSWORD_MASK and data.get("password") is None:
raise ValidationError("Must provide a password for the database")
+ @validates_schema
+ def validate_ssh_tunnel_password(self, data: Dict[str, Any], **kwargs: Any) -> None:
+ """If ssh_tunnel has a masked password, password is required"""
+ uuid = data["uuid"]
+ existing = db.session.query(Database).filter_by(uuid=uuid).first()
+ if existing:
+ return
+
+ # Our DB has a ssh_tunnel in it
+ if ssh_tunnel := data.get("ssh_tunnel"):
+ if not is_feature_enabled("SSH_TUNNELING"):
+ raise SSHTunnelingNotEnabledError()
+ password = ssh_tunnel.get("password")
+ if password == PASSWORD_MASK:
+ raise ValidationError("Must provide a password for the ssh tunnel")
+ return
+
+ @validates_schema
+ def validate_ssh_tunnel_private_key(
+ self, data: Dict[str, Any], **kwargs: Any
+ ) -> None:
+ """If ssh_tunnel has a masked private key, private key is required"""
+ uuid = data["uuid"]
+ existing = db.session.query(Database).filter_by(uuid=uuid).first()
+ if existing:
+ return
+
+ # Our DB has a ssh_tunnel in it
+ if ssh_tunnel := data.get("ssh_tunnel"):
+ if not is_feature_enabled("SSH_TUNNELING"):
+ raise SSHTunnelingNotEnabledError()
+ private_key = ssh_tunnel.get("private_key")
+ if private_key == PASSWORD_MASK:
+ raise ValidationError("Must provide a private key for the ssh tunnel")
Review Comment:
can we add a test to make sure this will raise when the user is missing the private key
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] codecov[bot] commented on pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23099:
URL: https://github.com/apache/superset/pull/23099#issuecomment-1433571718
# [Codecov](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23099](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6c44c8f) into [master](https://codecov.io/gh/apache/superset/commit/f4ffed24babc8923cc812bc7bf3cb07ffd7e367a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f4ffed2) will **decrease** coverage by `11.15%`.
> The diff coverage is `23.07%`.
> :exclamation: Current head 6c44c8f differs from pull request most recent head 0e653b7. Consider uploading reports for the commit 0e653b7 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23099 +/- ##
===========================================
- Coverage 67.54% 56.40% -11.15%
===========================================
Files 1881 1881
Lines 72392 72474 +82
Branches 7882 7879 -3
===========================================
- Hits 48898 40878 -8020
- Misses 21472 29575 +8103
+ Partials 2022 2021 -1
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.66% <15.38%> (-0.10%)` | :arrow_down: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.57% <15.38%> (-0.10%)` | :arrow_down: |
| python | `59.10% <23.07%> (-23.17%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `52.39% <23.07%> (-0.08%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rset-frontend/src/components/ImportModal/index.tsx](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSW1wb3J0TW9kYWwvaW5kZXgudHN4) | `73.41% <ø> (+0.24%)` | :arrow_up: |
| [superset-frontend/src/pages/ChartList/index.tsx](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3BhZ2VzL0NoYXJ0TGlzdC9pbmRleC50c3g=) | `54.61% <ø> (ø)` | |
| [...rontend/src/views/CRUD/dashboard/DashboardList.tsx](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGFzaGJvYXJkL0Rhc2hib2FyZExpc3QudHN4) | `54.86% <ø> (ø)` | |
| [...c/views/CRUD/data/database/DatabaseModal/index.tsx](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL2luZGV4LnRzeA==) | `45.69% <ø> (+0.39%)` | :arrow_up: |
| [...ontend/src/views/CRUD/data/dataset/DatasetList.tsx](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0RhdGFzZXRMaXN0LnRzeA==) | `56.77% <ø> (ø)` | |
| [.../src/views/CRUD/data/savedquery/SavedQueryList.tsx](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9zYXZlZHF1ZXJ5L1NhdmVkUXVlcnlMaXN0LnRzeA==) | `63.30% <ø> (ø)` | |
| [superset-frontend/src/views/CRUD/hooks.ts](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvaG9va3MudHM=) | `47.69% <ø> (ø)` | |
| [superset-frontend/src/views/CRUD/utils.tsx](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvdXRpbHMudHN4) | `67.39% <ø> (-0.24%)` | :arrow_down: |
| [superset/charts/api.py](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY2hhcnRzL2FwaS5weQ==) | `49.81% <0.00%> (-36.38%)` | :arrow_down: |
| [superset/commands/importers/v1/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL19faW5pdF9fLnB5) | `38.70% <0.00%> (-52.82%)` | :arrow_down: |
| ... and [308 more](https://codecov.io/gh/apache/superset/pull/23099?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] eschutho merged pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #23099:
URL: https://github.com/apache/superset/pull/23099
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] Antonio-RiveroMartnez commented on a diff in pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #23099:
URL: https://github.com/apache/superset/pull/23099#discussion_r1112312284
##########
superset/databases/schemas.py:
##########
@@ -719,6 +723,65 @@ def validate_password(self, data: Dict[str, Any], **kwargs: Any) -> None:
if password == PASSWORD_MASK and data.get("password") is None:
raise ValidationError("Must provide a password for the database")
+ @validates_schema
+ def validate_ssh_tunnel_password(self, data: Dict[str, Any], **kwargs: Any) -> None:
+ """If ssh_tunnel has a masked password, password is required"""
+ uuid = data["uuid"]
+ existing = db.session.query(Database).filter_by(uuid=uuid).first()
+ if existing:
+ return
+
+ # Our DB has a ssh_tunnel in it
+ if ssh_tunnel := data.get("ssh_tunnel"):
+ if not is_feature_enabled("SSH_TUNNELING"):
+ raise SSHTunnelingNotEnabledError()
+ password = ssh_tunnel.get("password")
+ if password == PASSWORD_MASK:
+ raise ValidationError("Must provide a password for the ssh tunnel")
+ return
+
+ @validates_schema
+ def validate_ssh_tunnel_private_key(
+ self, data: Dict[str, Any], **kwargs: Any
+ ) -> None:
+ """If ssh_tunnel has a masked private key, private key is required"""
+ uuid = data["uuid"]
+ existing = db.session.query(Database).filter_by(uuid=uuid).first()
+ if existing:
+ return
+
+ # Our DB has a ssh_tunnel in it
+ if ssh_tunnel := data.get("ssh_tunnel"):
+ if not is_feature_enabled("SSH_TUNNELING"):
+ raise SSHTunnelingNotEnabledError()
+ private_key = ssh_tunnel.get("private_key")
+ if private_key == PASSWORD_MASK:
+ raise ValidationError("Must provide a private key for the ssh tunnel")
Review Comment:
After re-reading this, I'm adding some more checks, for example as you mentioned, not only raising when `== PASSWORD_MASK` but also checking there's at least one login method (`password` OR `private_key + private_key_password`). That way, if users manipulate files manually, we still check the integrity of the data being passed. Thanks for the insight 👍
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] Antonio-RiveroMartnez commented on pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on PR #23099:
URL: https://github.com/apache/superset/pull/23099#issuecomment-1434101085
@eschutho Thanks for reminding me about such check 👍 . It should reject the operation and show an error message to the user in such case. Let me add that validation and its 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] Antonio-RiveroMartnez commented on a diff in pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #23099:
URL: https://github.com/apache/superset/pull/23099#discussion_r1110260291
##########
superset/databases/schemas.py:
##########
@@ -719,6 +723,65 @@ def validate_password(self, data: Dict[str, Any], **kwargs: Any) -> None:
if password == PASSWORD_MASK and data.get("password") is None:
raise ValidationError("Must provide a password for the database")
+ @validates_schema
+ def validate_ssh_tunnel_password(self, data: Dict[str, Any], **kwargs: Any) -> None:
+ """If ssh_tunnel has a masked password, password is required"""
+ uuid = data["uuid"]
+ existing = db.session.query(Database).filter_by(uuid=uuid).first()
+ if existing:
+ return
+
+ # Our DB has a ssh_tunnel in it
+ if ssh_tunnel := data.get("ssh_tunnel"):
+ if not is_feature_enabled("SSH_TUNNELING"):
+ raise SSHTunnelingNotEnabledError()
+ password = ssh_tunnel.get("password")
+ if password == PASSWORD_MASK:
+ raise ValidationError("Must provide a password for the ssh tunnel")
+ return
+
+ @validates_schema
+ def validate_ssh_tunnel_private_key(
+ self, data: Dict[str, Any], **kwargs: Any
+ ) -> None:
+ """If ssh_tunnel has a masked private key, private key is required"""
+ uuid = data["uuid"]
+ existing = db.session.query(Database).filter_by(uuid=uuid).first()
+ if existing:
+ return
+
+ # Our DB has a ssh_tunnel in it
+ if ssh_tunnel := data.get("ssh_tunnel"):
+ if not is_feature_enabled("SSH_TUNNELING"):
+ raise SSHTunnelingNotEnabledError()
+ private_key = ssh_tunnel.get("private_key")
+ if private_key == PASSWORD_MASK:
+ raise ValidationError("Must provide a private key for the ssh tunnel")
Review Comment:
Hey! I added a few in the `api_tests` and `command_tests` files:
https://github.com/apache/superset/pull/23099/files#diff-023d03cc917d159e443cf0e947dda5e28cfdf7c5a26c592860e1d419fc5b317eR651
https://github.com/apache/superset/pull/23099/files#diff-b94e273d2b41d3fe8a552cbf34cfa59ab9495154d16823b411470ae782cea38aR2473
Let me know if you consider we need to cover other scenarios in other places. 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] eschutho commented on pull request #23099: feat(ssh_tunnel): Import/Export Databases with SSHTunnel credentials
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #23099:
URL: https://github.com/apache/superset/pull/23099#issuecomment-1433841763
@Antonio-RiveroMartnez what happens when you try to import a db with ssh tunneling to an application that doesn't have the ssh tunnel feature flag turned on?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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