fix: add config to disable dataset ownership on the old api #13051
Conversation
| @@ -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) | |||
There was a problem hiding this comment.
Needed fix, sed was only replacing one non alphanumeric character
| @@ -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 | |||
There was a problem hiding this comment.
Let's also deprecate this configuration flag for 2.0
There was a problem hiding this 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
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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.
superset/views/datasource.py
Outdated
| @@ -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"] | |||
There was a problem hiding this comment.
nit: I think we want to keep the part of this that sets datasource_dict["owners"] and only disable the check_ownership function
dpgaspar
deleted the
danielgaspar/ch5442/add-config-key-to-ignore-check-ownership
branch
…3051) * fix: add config to disable dataset ownership on the old api * fix CI docker build * fix logic * add deprecation comment on the config
* master: (30 commits) refactor(native-filters): decouple params from filter config modal (first phase) (apache#13021) fix(native-filters): set currentValue null when empty (apache#13000) Custom superset_config.py + secret envs (apache#13096) Update http error code from 400 to 403 (apache#13061) feat(native-filters): add storybook entry for select filter (apache#13005) feat(native-filters): Time native filter (apache#12992) Force pod restart on config changes (apache#13056) feat(cross-filters): add cross filters (apache#12662) fix(explore): Enable selecting an option not included in suggestions (apache#13029) Improves RTL configuration (apache#13079) Added a note about the ! prefix for breaking changes to CONTRIBUTING.md (apache#13083) chore: lock down npm to v6 (apache#13069) fix: API tests, make them possible to run independently again (apache#13076) fix: add config to disable dataset ownership on the old api (apache#13051) add required * indicator to message content/notif method (apache#12931) fix: Retroactively add granularity param to charts (apache#12960) fix(ci): multiline regex in change detection (apache#13075) feat(style): hide dashboard header by url parameter (apache#12918) fix(explore): pie chart label bugs (apache#13052) fix: Disabled state button transition time (apache#13008) ...
mistercrunch
added
🏷️ bot
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_OWNERSHIPthat user's can set toFalseto return to the previous behaviour (no ownership checking).Also added a deprecation warning to the MVC endpoints and
datasource/save. Note that the new API for datasets will check for ownership and ignoreOLD_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