-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Cs/sc 3688 deploy updates #21
base: master
Are you sure you want to change the base?
Conversation
@@ -54,7 +54,8 @@ FEATURE_FLAGS = { | |||
"DASHBOARD_NATIVE_FILTERS_SET": True, | |||
"DASHBOARD_FILTERS_EXPERIMENTAL": True, | |||
"ENABLE_JAVASCRIPT_CONTROLS": True, | |||
"ENABLE_TEMPLATE_PROCESSING": True | |||
"ENABLE_TEMPLATE_PROCESSING": True, | |||
"ALLOW_ADHOC_SUBQUERY": True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This FF was introduced in superset version 3 and is basically just old code from version 2 put behind a FF for security reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we continue to support this over time?
# Any project which wants to add custom images to their dashboards must have | ||
# the url of the image source added here, e.g. "https://mycoolimagesite.com" | ||
|
||
TALISMAN_CONFIG = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config was copied from the superset codebase and tweaked slightly for our own uses.
The TALISMAN_ENABLED flag was set to a default True
value since superset 3 for security reasons (this flag enables content security policy checking). Likewise, the TALISMAN_CONFIG is the policy configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to simply edit the policies we're interested in, but it didn't seem to work quite right. I suspect whatever we set overrides the default variable instead of appending to it.
@Charl1996 |
@mkangia |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to merge considering this is already done on prod. Though for allow_adhoc_subquery
, I would like to understand if its safe in long run or we should take a note of it and/or discontinue it by letting projects know.
Good question. The FF seems stable and row level security is being applied, so at least for now we should be OK using it I think. What would make this FF unsafe in the long run do you think? |
If this was removed from Superset, I assume they had some concern with it? |
Ah, fair enough. |
I think I'll pause a little on merging this then. I want to see if I can read up a bit more around the specific security implications. |
This PR contains some updates made from this ticket.