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 2022/07/28 09:01:30 UTC
[GitHub] [superset] fbalicchia opened a new pull request, #20898: feat: fallback to external password store for sqlalchemy connections
fbalicchia opened a new pull request, #20898:
URL: https://github.com/apache/superset/pull/20898
### SUMMARY
Superset can be configured to use an external store for database passwords. This is useful if you a running a custom secret distribution framework and do not wish to store secrets in Superset’s meta database.
This useful capability can be extended by providing Superset’s meta database as a fallback in the case the administration doesn't want to use an external store for a particular database
### TESTING INSTRUCTIONS
Set `SQLALCHEMY_CUSTOM_PASSWORD_STORE` to point to an external lookup function. and the user does not have to check where her password is saved. Current tests must continue to work
### ADDITIONAL INFORMATION
- [ ] 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
- [ ] 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
Re: [PR] feat: fallback to external password store for sqlalchemy connections [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #20898:
URL: https://github.com/apache/superset/pull/20898#issuecomment-1930668619
Sorry this slid under everyone's radar for so long. Adding @dpgaspar for a security perspective, and closing/reopening to reboot the CI process.
--
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 #20898: feat: fallback to external password store for sqlalchemy connections
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20898:
URL: https://github.com/apache/superset/pull/20898#issuecomment-1197891283
# [Codecov](https://codecov.io/gh/apache/superset/pull/20898?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 [#20898](https://codecov.io/gh/apache/superset/pull/20898?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e0ca3b4) into [master](https://codecov.io/gh/apache/superset/commit/718bc3062e99cc44afbb57f786b5ca228c5b13fb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (718bc30) will **decrease** coverage by `0.08%`.
> The diff coverage is `77.77%`.
```diff
@@ Coverage Diff @@
## master #20898 +/- ##
==========================================
- Coverage 66.27% 66.18% -0.09%
==========================================
Files 1758 1758
Lines 67073 67079 +6
Branches 7122 7122
==========================================
- Hits 44453 44399 -54
- Misses 20801 20861 +60
Partials 1819 1819
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `53.25% <44.44%> (-0.01%)` | :arrow_down: |
| mysql | `80.99% <77.77%> (-0.01%)` | :arrow_down: |
| postgres | `?` | |
| presto | `53.14% <44.44%> (-0.01%)` | :arrow_down: |
| python | `81.37% <77.77%> (-0.19%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `?` | |
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/20898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.36% <77.77%> (-0.32%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/sql\_validators/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvX19pbml0X18ucHk=) | `80.00% <0.00%> (-20.00%)` | :arrow_down: |
| [superset/sqllab/sql\_json\_executer.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL3NxbF9qc29uX2V4ZWN1dGVyLnB5) | `64.63% <0.00%> (-9.76%)` | :arrow_down: |
| [superset/sqllab/execution\_context\_convertor.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL2V4ZWN1dGlvbl9jb250ZXh0X2NvbnZlcnRvci5weQ==) | `81.48% <0.00%> (-7.41%)` | :arrow_down: |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `90.67% <0.00%> (-5.94%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `91.89% <0.00%> (-5.41%)` | :arrow_down: |
| [superset/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9sb2dfcHJ1bmUucHk=) | `85.71% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3V0aWxzLnB5) | `86.36% <0.00%> (-2.28%)` | :arrow_down: |
| ... and [10 more](https://codecov.io/gh/apache/superset/pull/20898/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?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] fbalicchia commented on pull request #20898: feat: fallback to external password store for sqlalchemy connections
Posted by GitBox <gi...@apache.org>.
fbalicchia commented on PR #20898:
URL: https://github.com/apache/superset/pull/20898#issuecomment-1317324502
Hi @hughhhh I'm sorry if I link directly but I saw that you have some commit on this piece of code. Could you please review this pr and if it has make sense to you
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] john-bodley commented on a diff in pull request #20898: feat: fallback to external password store for sqlalchemy connections
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #20898:
URL: https://github.com/apache/superset/pull/20898#discussion_r1026869963
##########
superset/models/core.py:
##########
@@ -340,9 +340,14 @@ def get_password_masked_url(cls, masked_url: URL) -> URL:
def set_sqlalchemy_uri(self, uri: str) -> None:
conn = make_url_safe(uri.strip())
- if conn.password != PASSWORD_MASK and not custom_password_store:
- # do not over-write the password with the password mask
- self.password = conn.password
+ input_password = conn.password
Review Comment:
It seems like this logic could be simplified, i.e.,
```python
if (
custom_password_store
and custom_password_store(conn) is None
or not custom_password_store
and conn.password != PASSWORD_MASK
):
self.password = conn.password
```
##########
superset/models/core.py:
##########
@@ -739,7 +744,11 @@ def sqlalchemy_uri_decrypted(self) -> str:
# (so users see 500 less often)
return "dialect://invalid_uri"
if custom_password_store:
- conn = conn.set(password=custom_password_store(conn))
+ password_to_check = custom_password_store(conn)
Review Comment:
Similarly,
```python
conn = conn.set(password=custom_password_store(conn) or self.password)
```
##########
tests/integration_tests/core_tests.py:
##########
@@ -597,6 +597,22 @@ def custom_password_store(uri):
# Disable for password store for later tests
models.custom_password_store = None
+ def test_custom_password_store_fallback(self):
+ database = superset.utils.database.get_example_database()
+ conn_pre = sqla.engine.url.make_url(database.sqlalchemy_uri_decrypted)
+
+ def custom_password_store(uri):
+ return None
+
+ models.custom_password_store = custom_password_store
+ conn = sqla.engine.url.make_url(database.sqlalchemy_uri_decrypted)
+ database.set_sqlalchemy_uri(database.sqlalchemy_uri_decrypted)
+ if conn_pre.password:
Review Comment:
If this is false then the assertions would never execute making the test superfluous. Is this check required?
--
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] fbalicchia commented on a diff in pull request #20898: feat: fallback to external password store for sqlalchemy connections
Posted by GitBox <gi...@apache.org>.
fbalicchia commented on code in PR #20898:
URL: https://github.com/apache/superset/pull/20898#discussion_r1029182850
##########
tests/integration_tests/core_tests.py:
##########
@@ -597,6 +597,22 @@ def custom_password_store(uri):
# Disable for password store for later tests
models.custom_password_store = None
+ def test_custom_password_store_fallback(self):
+ database = superset.utils.database.get_example_database()
+ conn_pre = sqla.engine.url.make_url(database.sqlalchemy_uri_decrypted)
+
+ def custom_password_store(uri):
+ return None
+
+ models.custom_password_store = custom_password_store
+ conn = sqla.engine.url.make_url(database.sqlalchemy_uri_decrypted)
+ database.set_sqlalchemy_uri(database.sqlalchemy_uri_decrypted)
+ if conn_pre.password:
Review Comment:
The scope of None is to say to the system to use an input password instead of a connection password
##########
superset/models/core.py:
##########
@@ -340,9 +340,14 @@ def get_password_masked_url(cls, masked_url: URL) -> URL:
def set_sqlalchemy_uri(self, uri: str) -> None:
conn = make_url_safe(uri.strip())
- if conn.password != PASSWORD_MASK and not custom_password_store:
- # do not over-write the password with the password mask
- self.password = conn.password
+ input_password = conn.password
Review Comment:
Hi, first of all, thanks. I agree with you that can be necessary to make the logic more simple the logic and clear the code,
the first and the second have two different purposes cause if the external function return None
system needs to use an input password and not `conn.password` but probably I'm missing something from you tips
--
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
Re: [PR] feat: fallback to external password store for sqlalchemy connections [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #20898: feat: fallback to external password store for sqlalchemy connections
URL: https://github.com/apache/superset/pull/20898
--
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