john-bodley opened a new pull request, #20617: URL: https://github.com/apache/superset/pull/20617
<!--- Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/ Example: fix(dashboard): load charts correctly --> ### SUMMARY https://github.com/apache/superset/pull/19854 introduced frontend logic which would check that (for Alpha users) only owners can edit datasets. The issue is the definition of ownership between the frontend and backend differs. Specifically the frontend merely checks whether the user is listed as a owner—per the [owners](https://github.com/apache/superset/blob/master/superset/connectors/sqla/models.py#L694) relationship—whereas the backend uses the [check_ownership](https://github.com/apache/superset/blob/3483446c28113fb9eb1e8adb8978e305ff5e7c65/superset/views/base.py#L647-L691) method which checks (in addition to the `owner` and `owners` relationships) whether the user is the creator. This dichotomy means that creators who are not explicitly listed as owners are unable to edit their datasets. Unwinding the logic surfaces additional observations regarding ownership for various assets, i.e., charts, dashboards, datasets, and reports: 1. The DAO logic correctly adds the creator as an explicit owner ([example](https://github.com/apache/superset/blob/master/superset/datasets/commands/create.py#L92-L93)) so this isn't an issue for any asset which was created via a DAO commend. 2. Prior to the DAO logic only dashboards contained [pre_add](https://github.com/apache/superset/blob/8e29ec5a6685867ffc035d20999c54c2abe36fb1/superset/views/dashboard/views.py#L96-L97) logic which added the creator as an explicit owner meaning that this issue only impacts historical charts and datasets. 3. The `check_ownership` method contained a check for an `owner` relationship however grepping the code per `git grep "\bowner = relationship\b"` yielded no matches, i.e., it served no purpose. Given these insights this PR performs the following: 1. It adds a database migration to add all creators as owners (if not already listed) for both charts and datasets. Granted this may be a little presumptive, i.e., we may be re-adding someone as an "owner" who was previously removed, however there's no way to determine what the intent of the current database state is. 2. It updates the `check_ownership` method to remove checking the `owner` (obsolete) and `created_by` (addressed in the migration) fields. Finally a few things worth noting: 1. The frontend likely should rely on an API call to check ownership to adhere to the DRY principle and ensure that the frontend and backend logic for ownership is consistent. 2. The backend `pre_add`, `pre_update`, etc. checks should likely be deprecated, i.e., for dashboards the [pre_add](https://github.com/apache/superset/blob/8e29ec5a6685867ffc035d20999c54c2abe36fb1/superset/views/dashboard/views.py#L90-L102) logic differs fairly significantly from the [DAO](https://github.com/apache/superset/blob/master/superset/dashboards/commands/create.py#L52-L80) logic both in terms of slugs and ownership propagation, i.e., for right or wrong the `pre_add` also makes the dashboard editor an owner of all the associated slices whereas this isn't the case for the DAO command. This really muddies the code logic and makes it hard to grok how things work (or should work). 3. I'm not really sure why the DAO `validate` command also populates fields, i.e., adds owners. This seems somewhat contradictory and non-intuitive, i.e., validation shouldn't update the model. I gather it was likely for efficiency reasons, but I sense there is merit in rewriting the DAO logic to break up the validation and population phases. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF <!--- Skip this if not applicable --> ### TESTING INSTRUCTIONS Tested the migration and confirmed that, ```sql SELECT s.id, s.created_by_fk FROM slices s LEFT OUTER JOIN slice_user su ON s.id = su.slice_id AND s.created_by_fk = su.user_id WHERE su.slice_id IS NULL ``` and ```sql SELECT t.id, t.created_by_fk FROM tables t LEFT OUTER JOIN sqlatable_user su ON t.id = su.table_id AND t.created_by_fk = su.user_id WHERE su.table_id IS NULL ``` returned no rows. ### 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 - [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [x] Migration is atomic, supports rollback & is backwards-compatible - [x] 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
