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 2021/02/10 10:57:50 UTC
[GitHub] [superset] dpgaspar opened a new pull request #13051: fix: add config to disable dataset ownership on the old api
dpgaspar opened a new pull request #13051:
URL: https://github.com/apache/superset/pull/13051
### SUMMARY
A follow up for #12491 that caused problems. The ownership check is inline with the current security logic that only owners can change their object metadata.
This PR adds a new config flag `OLD_API_CHECK_DATASET_OWNERSHIP` where user's can set to `False` to return to previous behaviour. Without checking the ownership.
Also added a deprecation warning to the MVC endpoints and `datasource/save`. Note that the new API for datasets will check for ownership and ignore `OLD_API_CHECK_DATASET_OWNERSHIP`.
Yet, probably the ownership concept is problematic, and a move for using associated roles to objects (dashboards, datasets, charts) is easier to manage.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] 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.
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] etr2460 commented on a change in pull request #13051: fix: add config to disable dataset ownership on the old api
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #13051:
URL: https://github.com/apache/superset/pull/13051#discussion_r574126735
##########
File path: superset/config.py
##########
@@ -1057,6 +1057,10 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
'class="alert-link">here</a>.'
)
+# Turn this key to False to disable ownership check on the old dataset MVC and
+# datasource API /datasource/save.
+OLD_API_CHECK_DATASET_OWNERSHIP = True
Review comment:
Providing a group could fix this, as long as it's easy to make sure everyone is in that group (not sure what group means in this case, are we talking about a role like `Alpha` or `Gamma`?).
Basically we rely on this because many physical datasets are used by many people on many teams (think a table like `bookings`). Many people would want to build charts based off this table, and make calculated columns or metrics that could be shared across other charts or to other people (maybe there's a calculated column called `is_weekend` that does logic based on the dttm the booking is). We want to enable and encourage this behavior, otherwise we'd end up in a situation where there are dozens of physical datasets for `bookings`, each with different metrics and calculated columns (and even worse, different definitions of the same metrics or calculated columns).
Finally, while we do want everyone to be able to take some actions on a dataset (adding metrics, calculated columns, better column descriptions, etc.) it would be nice to restrict other actions to owners only (deleting the dataset, changing the SQL of a virtual dataset). In an ideal world, we'd have more fine grain permission on datasets that could allow anyone to do non-destructive actions, but only let owners/admins perform destructive ones.
----------------------------------------------------------------
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] [superset] willbarrett commented on a change in pull request #13051: fix: add config to disable dataset ownership on the old api
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #13051:
URL: https://github.com/apache/superset/pull/13051#discussion_r573906253
##########
File path: superset/config.py
##########
@@ -1057,6 +1057,10 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
'class="alert-link">here</a>.'
)
+# Turn this key to False to disable ownership check on the old dataset MVC and
+# datasource API /datasource/save.
+OLD_API_CHECK_DATASET_OWNERSHIP = True
Review comment:
Let's also deprecate this configuration flag for 2.0
----------------------------------------------------------------
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] [superset] dpgaspar commented on a change in pull request #13051: fix: add config to disable dataset ownership on the old api
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #13051:
URL: https://github.com/apache/superset/pull/13051#discussion_r573641355
##########
File path: .github/workflows/docker_build_push.sh
##########
@@ -21,14 +21,14 @@ SHA=$(git rev-parse HEAD)
REPO_NAME="apache/superset"
if [[ "${GITHUB_EVENT_NAME}" == "pull_request" ]]; then
- REFSPEC=$(echo "${GITHUB_HEAD_REF}" | sed 's/[^a-zA-Z0-9]/-/' | head -c 40)
+ REFSPEC=$(echo "${GITHUB_HEAD_REF}" | sed 's/[^a-zA-Z0-9]/-/g' | head -c 40)
Review comment:
Needed fix, `sed` was only replacing one non alphanumeric character
----------------------------------------------------------------
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] [superset] etr2460 commented on a change in pull request #13051: fix: add config to disable dataset ownership on the old api
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #13051:
URL: https://github.com/apache/superset/pull/13051#discussion_r573959851
##########
File path: superset/config.py
##########
@@ -1057,6 +1057,10 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
'class="alert-link">here</a>.'
)
+# Turn this key to False to disable ownership check on the old dataset MVC and
+# datasource API /datasource/save.
+OLD_API_CHECK_DATASET_OWNERSHIP = True
Review comment:
If this is deprecated in 2.0, do we think we'll have another way to allow non-owners to edit datasets (perhaps only allowing non-destructive changes)? The concept of shared physical datasets is something that's pretty relied on at Airbnb, and I think is a "good product feature" since it prevents duplication of datasets and ensures everyone is using the same source of truth
----------------------------------------------------------------
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] [superset] etr2460 commented on a change in pull request #13051: fix: add config to disable dataset ownership on the old api
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #13051:
URL: https://github.com/apache/superset/pull/13051#discussion_r574131362
##########
File path: superset/views/datasource.py
##########
@@ -53,7 +53,11 @@ def save(self) -> FlaskResponse:
)
orm_datasource.database_id = database_id
- if "owners" in datasource_dict and orm_datasource.owner_class is not None:
+ if (
+ app.config["OLD_API_CHECK_DATASET_OWNERSHIP"]
Review comment:
nit: I think we want to keep the part of this that sets `datasource_dict["owners"]` and only disable the `check_ownership` function
----------------------------------------------------------------
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] [superset] willbarrett commented on a change in pull request #13051: fix: add config to disable dataset ownership on the old api
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #13051:
URL: https://github.com/apache/superset/pull/13051#discussion_r574134278
##########
File path: superset/config.py
##########
@@ -1057,6 +1057,10 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
'class="alert-link">here</a>.'
)
+# Turn this key to False to disable ownership check on the old dataset MVC and
+# datasource API /datasource/save.
+OLD_API_CHECK_DATASET_OWNERSHIP = True
Review comment:
Erik and I discussed a bit out of band - I think we're going to explore the topic of the correct behavior here and bring a proposal back to the community. For now, let's consider this conversation non-blocking for this PR.
----------------------------------------------------------------
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] [superset] dpgaspar merged pull request #13051: fix: add config to disable dataset ownership on the old api
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #13051:
URL: https://github.com/apache/superset/pull/13051
----------------------------------------------------------------
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] [superset] willbarrett commented on a change in pull request #13051: fix: add config to disable dataset ownership on the old api
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #13051:
URL: https://github.com/apache/superset/pull/13051#discussion_r574103505
##########
File path: superset/config.py
##########
@@ -1057,6 +1057,10 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
'class="alert-link">here</a>.'
)
+# Turn this key to False to disable ownership check on the old dataset MVC and
+# datasource API /datasource/save.
+OLD_API_CHECK_DATASET_OWNERSHIP = True
Review comment:
Hrm... @etr2460 before this point, my understanding was that this was a hole in our RBAC configuration, and what this flag was for was to allow organizations time to migrate their existing data. If this is a requirement on Superset going forward then I think it would be good to discuss it. @dpgaspar mentioned that he thinks eventually migrating to a group permission rather than an individual permission could accommodate this need - anyone in a specified group would be able to edit the datasource, rather than a list of owners. Can you talk more about how this is relied on at Airbnb? Would providing a group as the "owner" of the datasource resolve 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